This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] trunc (fptoui|fptosi)
ClosedPublic

Authored by samparker on Jan 19 2023, 2:03 AM.

Details

Summary

Attempt to fold the trunc into the fp-to-int conversion.

https://alive2.llvm.org/ce/z/8RCNou

Diff Detail

Event Timeline

samparker created this revision.Jan 19 2023, 2:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 2:03 AM
samparker requested review of this revision.Jan 19 2023, 2:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 2:03 AM
samparker updated this revision to Diff 490438.Jan 19 2023, 3:09 AM
samparker edited the summary of this revision. (Show Details)

Now only checking for poison/undef for the signed case.

samparker added inline comments.Jan 19 2023, 3:33 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
506

And I'm still not sure whether this is needed? Alive seems to want it to be happy, but I pretty sure integer transforms are performed elsewhere without considering fp-to-int conversions as inputs.

I realize it's unlikely in practice, but is there a reason not to support any FP type? For fptoui, the integer type just needs one more bit than the max exponent for a given FP semantic?

For the tests, it should be sufficient to have the intermediate integer width be one more than the minimum required type width, so "%i = fptoui half %x to i17".

Please pre-commit baseline tests (either locally or push to main) and label tests that should not change as negative tests (either in the function name or with a code comment).

it should be sufficient to have the intermediate integer width be one more than the minimum required type width, so "%i = fptoui half %x to i17".

IIUC, for half fptoui we don't need an i17, as an i16 can hold the max normal value (65504). I can add support in for float conversions though, as this logic is only triggering for simple types, I assume the only conversion that will work is float -> i128 -> i64.

I would really appreciate if someone could help me understand the complication with fptosi w.r.t checking for poison/undef too.

nikic added a subscriber: nikic.Jan 20 2023, 3:31 AM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
506

Can you share the problematic proof? It shouldn't be needed.

samparker added inline comments.Jan 20 2023, 4:14 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
506

It doesn't make sense to me, but I'm hopeless with FP, so just alive... https://alive2.llvm.org/ce/z/fr5kdx. It will only compile with --disable-undef-input or a noundef operand.

it should be sufficient to have the intermediate integer width be one more than the minimum required type width, so "%i = fptoui half %x to i17".

IIUC, for half fptoui we don't need an i17, as an i16 can hold the max normal value (65504). I can add support in for float conversions though, as this logic is only triggering for simple types, I assume the only conversion that will work is float -> i128 -> i64.

i16 is the smallest final type for fptoui; i17 is one bit bigger because we need to truncate at least one bit. We don't really want a "simple type" limit here in IR either unless there's some codegen concern. I'd add tests with float and bfloat. There's a current discussion about adding various other small format FP types to IR, so we should try to future-proof this transform for those types in case they make it into IR.

I would really appreciate if someone could help me understand the complication with fptosi w.r.t checking for poison/undef too.

There is no undef problem - I think it's just that the online instance times out with larger widths:
https://alive2.llvm.org/ce/z/6EZXLQ

Okay, great. Thanks for clarification on both fronts. I'm just about to commit some tests.

samparker updated this revision to Diff 490840.Jan 20 2023, 7:18 AM

Removed non-poison input restriction for fptosi.

efriedma added inline comments.Jan 20 2023, 11:00 AM
llvm/test/Transforms/InstCombine/trunc-fp-to-int.ll
357

From alive2:

define i33 @src(float noundef %x) {
%0:
  %conv = fptosi float noundef %x to i64
  %conv.1 = trunc i64 %conv to i33
  ret i33 %conv.1
}
=>
define i33 @tgt(float noundef %x) {
%0:
  %conv = fptosi float noundef %x to i33
  ret i33 %conv
}
Transformation doesn't verify!

ERROR: Target is more poisonous than source

The final result type must be able to hold the largest finite number representable in the floating-point type; otherwise, the transform isn't legal. For float, that's an i128 or i129, I think?

AArch64 does have an instruction FJCVTZS you could theoretically use for this kind of thing, but that seems unlikely to be worthwhile.

I updated the test file; see if this covers everything for fptoui:
cb29ba9c0f87
(if yes, then we duplicate each test for fptoui with one extra bit for the integer types)

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
503–506

This isn't correct - we want to use semanticsMaxExponent() or something like that to determine the minimum bitwidth. Signed cast needs one extra bit to not truncate the sign bit in integer form.

llvm/test/Transforms/InstCombine/trunc-fp-to-int.ll
160

This and the following tests are miscompiles (noundef is used here only to prevent the timeout):
https://alive2.llvm.org/ce/z/UCo_Py

samparker updated this revision to Diff 491313.Jan 23 2023, 5:03 AM
samparker edited the summary of this revision. (Show Details)

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.

samparker updated this revision to Diff 491329.Jan 23 2023, 5:46 AM

Avoiding integer comparison warning.

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().

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
502–503

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;
llvm/test/Transforms/InstCombine/trunc-fp-to-int.ll
43

Put a TODO comment on this since we don't do it yet.

119

Oops - yes, I typo'd the test names for doubles.

137

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.

samparker updated this revision to Diff 491340.Jan 23 2023, 6:20 AM

Rebased and added comment.

spatel accepted this revision.Jan 23 2023, 8:22 AM

LGTM

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
508

Formatting nit: variable should have a capitalized name.

This revision is now accepted and ready to land.Jan 23 2023, 8:22 AM
This revision was landed with ongoing or failed builds.Jan 24 2023, 1:16 AM
This revision was automatically updated to reflect the committed changes.