llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
307 stars 84 forks source link

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

Open Lancern opened 3 weeks ago

Lancern commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

Rebased onto the latest main.

bcardosolopes commented 2 weeks ago

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)

Lancern commented 1 week ago

Rebased onto the latest main.