Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIR][Fix] FP builtins should lower directly to LLVM builtins #670

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lancern
Copy link
Collaborator

@Lancern Lancern commented Jun 8, 2024

LLVM lowering for the following operations is introduced in #616 and #651: cos, exp, exp2, log, log10, log2, sin, sqrt, fmod, and pow. However, they are not lowered to their corresponding LLVM intrinsics; instead they are transformed to libc calls during lowering prepare. This does not match the upstream behavior.

This PR tries to correct this mistake. It makes all CIR FP intrinsic ops lower to their corresponding LLVM intrinsics (fmod is a special case and it is lowered to the frem LLVM instruction).

@Lancern
Copy link
Collaborator Author

Lancern commented Jun 8, 2024

Let me illustrate more background here so the motivation of this change can be made clearer. Consider the following code:

double test(double x) {
  return __builtin_cos(x);
}

Compile it w/o clangir and we get:

define dso_local double @test(double noundef %x) {
entry:
  ;; ...
  %call = call double @cos(double noundef %0) #3
  ;; ... 
}

This is why the two patches mentioned above decide to convert those FP builtins to libc calls during lowering prepare. But if you do some debugging work you'll find that __builtin_cos does not always correspond to cir.cos. It lowers to cir.cos only if -ffast-math is specified. When -ffast-math is not specified, CIRGen and CodeGen will directly generate a call to the cos function in libc.

So converting cir.cos to a libc call is incorrect behavior. This also applies to other operations mentioned above.

@bcardosolopes
Copy link
Member

you'll find that __builtin_cos does not always correspond to cir.cos. It lowers to cir.cos only if -ffast-math is specified. When -ffast-math is not specified, CIRGen and CodeGen will directly generate a call to the cos function in libc.

Nice catch!

@bcardosolopes
Copy link
Member

bcardosolopes commented Jun 10, 2024

It lowers to cir.cos only if -ffast-math is specified. When -ffast-math is not specified, CIRGen and CodeGen will directly generate a call to the cos function in libc.

Sounds like the previous approach still works for -ffast-math though, wouldn't it be best just to add tests for -ffast-math + fix a few lines of logic + take advantage of existing support you already put work on? Or putting in other words, after this PR, do we assert when -ffast-math is on?

@Lancern
Copy link
Collaborator Author

Lancern commented Jun 15, 2024

Or putting in other words, after this PR, do we assert when -ffast-math is on?

Do you mean we should assert for -ffast-math when we are CIRGen-ing the FP builtins listed here? Yes that would be nice and I'll add it later (maybe another PR).

@Lancern
Copy link
Collaborator Author

Lancern commented Jun 15, 2024

Rebased onto the latest main.

@bcardosolopes
Copy link
Member

Do you mean we should assert for -ffast-math when we are CIRGen-ing the FP builtins listed here? Yes that would be nice and I'll add it later (maybe another PR).

What I mean is that there's clearly a path that's now emitting wrong code and nothing is being done about it, please prevent that from happening as part of this PR, otherwise it might become technical debt - I suggest you add assertions or just add the proper hooks/support (so you don't need to delete a bunch of useful code you already added)

LLVM lowering for the following operations is introduced in llvm#616 and llvm#651: cos,
exp, exp2, log, log10, log2, sin, sqrt, fmod, and pow. However, they are not
lowered to their corresponding LLVM intrinsics; instead they are transformed to
libc calls during lowering prepare. This does not match the upstream behavior.

This patch tries to correct this mistake.
@Lancern
Copy link
Collaborator Author

Lancern commented Jun 24, 2024

Rebased onto the latest main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants