[PATCH] D125755: [AggressiveInstcombine] Conditionally fold saturated fptosi to llvm.fptosi.sat

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:405-407
+  SatCost += TTI.getCastInstrCost(Instruction::SExt, SatTy, IntTy,
+                                  TTI::CastContextHint::None,
+                                  TTI::TCK_RecipThroughput);
----------------
dnsampaio wrote:
> Hi  @dmgreen, long time.
> I'm updating our downstream compiler from llvm 12 to llvm 15 and this getCastInstrCost call is doing a rather strange thing, by asking the cost of  a sign extend from `i32` to `i8`, which seems awkward.
> Instead of using the fixed Instruction::SExt, shouldn't it be using `CastInst::getCastOpcode(IntTy, true, SatTy, true)` here?
> 
> Reproducer:
> ```
> @a = global float 0.000000e+00
> @b = global i32 0
> 
> define i32 @c() {
>   %1 = load float, ptr @a
>   %2 = fptosi float %1 to i32
>   %3 = call i32 @llvm.smin.i32(i32 %2, i32 127)
>   %4 = call i32 @llvm.smax.i32(i32 %3, i32 -128)
>   store i32 %4, ptr @b
>   ret i32 undef
> }
> 
> declare i32 @llvm.smin.i32(i32, i32)
> 
> declare i32 @llvm.smax.i32(i32, i32)
> ```
> 
> Coming from a creduced code:
> ```
> float a;
> b;
> c() {
>   b = a;
>   if (b > 127)
>     b = 127;
>   if (b < -128)
>     b = -128;
> }
> ```
> 
> 
Hello. Hope you are well.

It sounds like the dst and src types are the wrong way around. I think that SatTy should always be smaller than IntTy. I can run a couple of quick tests and will put up a patch for it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125755/new/

https://reviews.llvm.org/D125755