Page MenuHomePhabricator

Fix misfolding of IRBuilder.CreateICmp(int_ty X, bitcast (float_ty Y) to int_ty)
ClosedPublic

Authored by AndrewScheidecker on Aug 24 2018, 6:59 AM.

Details

Summary

This code was incorrectly folding:

IRBuilder.CreateICmp(int_ty X, bitcast (float_ty Y) to int_ty)

to:

IRBuilder.CreateICmp(bitcast (int_ty X) to float_ty, float_ty Y)

which is malformed IR due to the mismatch between the integer predicate and the FP operands. In my case, this triggered a crash in an optimization pass.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri added inline comments.
lib/IR/ConstantFold.cpp
1986–1994 ↗(On Diff #162373)

And what if the left side is FP? Or both of them?

lib/IR/ConstantFold.cpp
1986–1994 ↗(On Diff #162373)

That's a good question. That final else of the function seems to assume that it's folding an icmp (e.g. note the ConstantExpr::getICmp just below), but that is implicit in the control flow.

This branch handles the case where either operand is undef: https://github.com/llvm-mirror/llvm/blob/71589ff6720f9667b245f0cdb8a26f491c51e9fc/lib/IR/ConstantFold.cpp#L1714

This branch handles the case where both operands are constant floats: https://github.com/llvm-mirror/llvm/blob/71589ff6720f9667b245f0cdb8a26f491c51e9fc/lib/IR/ConstantFold.cpp#L1789

This branch handles the case where the operands are vectors of any type: https://github.com/llvm-mirror/llvm/blob/71589ff6720f9667b245f0cdb8a26f491c51e9fc/lib/IR/ConstantFold.cpp#L1832

And this branch handles the case where one or both the operands are floating-point ConstantExprs: https://github.com/llvm-mirror/llvm/blob/71589ff6720f9667b245f0cdb8a26f491c51e9fc/lib/IR/ConstantFold.cpp#L1850

So it should only be possible to reach that final else when the operands are scalar non-FP, assuming that there isn't a way to construct a Constant that has scalar FP type but isn't a ConstantFP or ConstantExpr.

I added assertions locally to verify that this if statement is only ever reached with an ICMP predicate, and with both operands having non-FP type, and they weren't triggered by "make check".

The ConstantExpr::getICmp call inside the if-then clause asserts if pred isn't an icmp predicate, and since an icmp with FP operands is invalid, that implies the original code would not handle a comparison with FP operands that got to this if statement.

I made the test a little simpler by bitcasting a GlobalVariable pointer to a float to get a FP typed ConstantExpr, instead of using a bitcast vector.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 11:49 AM

This took some work, but I have an IR test case that should show the bug:
rL353883

I haven't looked at D51216 yet, so this might be a dumb question, but is there an fcmp sibling to this bug?

unittests/IR/IRBuilderTest.cpp
679 ↗(On Diff #166177)

This means that IRBuilder has a bug/opportunity to be stricter? How did we create an integer add with FP operands?

This took some work, but I have an IR test case that should show the bug:
rL353883

Nice!

I haven't looked at D51216 yet, so this might be a dumb question, but is there an fcmp sibling to this bug?

There doesn't seem to be. I wrote a fcmp equivalent to the test you added to bitcast.ll in rL353883, and it behaved as expected.

unittests/IR/IRBuilderTest.cpp
679 ↗(On Diff #166177)

Oops, that should be CreateFAdd. It looks like this would trigger an assertion on a NDEBUG build. But should I keep this test if it's redundant with the IR test case you added?

spatel added inline comments.Feb 13 2019, 5:33 AM
unittests/IR/IRBuilderTest.cpp
679 ↗(On Diff #166177)

No - we can remove this test now. Please rebase/update with the new IR test.

Rebased, removed the superfluous IRBuilder test, updated the new IR test to reflect the fix, and added an IR test of the equivalent fcmp misfolding.

spatel accepted this revision.Feb 13 2019, 6:02 AM

LGTM

This revision is now accepted and ready to land.Feb 13 2019, 6:02 AM
This revision was automatically updated to reflect the committed changes.