[PATCH] D142093: [InstCombine] trunc (fptoui|fptosi)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D142093#4073138 <https://reviews.llvm.org/D142093#4073138>, @samparker wrote:

> Using semanticsMaxExponent, so hopefully correct now...
>
> A scalar trunc, to a non-simple type, is still only explored if the input is also a non-simple type but I presume that would be better changed, if at all, in a separate patch.

Yes, presumably that's a rarer possibility (and covered by the "wider final type" tests), but we'd need to ease the type check leading into canEvaluateTruncated().



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:502-503
+    Type *InputTy = I->getOperand(0)->getType()->getScalarType();
+    const fltSemantics &semantics = InputTy->getFltSemantics();
+    int32_t MinBitWidth = I->getOpcode() == Instruction::FPToSI
+                        ? APFloatBase::semanticsMaxExponent(semantics) + 1
----------------
This looks correct, but it's non-obvious, so it could use some explanatory code comments. Also, the code as written had a signed compare warning.

I'd rewrite it as something like:

```
    // If the integer type can hold the max FP value, it is safe to cast
    // directly to that type. Otherwise, we may create poison via overflow
    // that did not exist in the original code.
    //
    // The max FP value is pow(2, MaxExponent) * (1 + MaxFraction), so we need
    // at least one more bit than the MaxExponent to hold the max FP value.
    Type *InputTy = I->getOperand(0)->getType()->getScalarType();
    unsigned MinBitWidth =
        APFloat::semanticsMaxExponent(InputTy->getFltSemantics());

    // We need one more bit to preserve the signbit through truncation.
    if (I->getOpcode() == Instruction::FPToSI)
      ++MinBitWidth;
    return Ty->getScalarSizeInBits() > MinBitWidth;

```


================
Comment at: llvm/test/Transforms/InstCombine/trunc-fp-to-int.ll:43
 
 ; Wider final type is ok.
 
----------------
Put a TODO comment on this since we don't do it yet.


================
Comment at: llvm/test/Transforms/InstCombine/trunc-fp-to-int.ll:119
 
-define i1024 @float_fptoui_i1025_i1024(double %x) {
-; CHECK-LABEL: @float_fptoui_i1025_i1024(
----------------
Oops - yes, I typo'd the test names for doubles.


================
Comment at: llvm/test/Transforms/InstCombine/trunc-fp-to-int.ll:137
+
+; Negative test - not enough bits to hold min half value (-65504).
+
----------------
These look good - please pre-commit the tests with baseline results in a preliminary NFC patch.

We should add one more test with an extra use like this:

```
declare void @use(i129)

define i128 @float_fptoui_i129_i128_use(float %x) {
  %i = fptoui float %x to i129
  call void @use(i129 %i)
  %r = trunc i129 %i to i128
  ret i128 %r
}
```

We won't transform that currently, but we could allow that.



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

https://reviews.llvm.org/D142093