Page MenuHomePhabricator

[InstCombine] Don't assume CmpInst has been visited in getFlippedStrictnessPredicateAndConstant
ClosedPublic

Authored by bjope on Sep 25 2019, 7:15 AM.

Details

Summary

Removing an assumption (assert) that the CmpInst already has been
simplified in getFlippedStrictnessPredicateAndConstant. Solution is
to simply bail out instead of hitting the assertion. Instead we
assume that any profitable rewrite will happen in the next iteration
of InstCombine.

The reason why we can't assume that the CmpInst already has been
simplified is that the worklist does not guarantee such an ordering.

Solves https://bugs.llvm.org/show_bug.cgi?id=43376

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.Sep 25 2019, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 7:15 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
lebedev.ri accepted this revision.Sep 25 2019, 7:29 AM

LG, sorry for the breakage!

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
5139–5140 ↗(On Diff #221762)
if (auto* CI = dyn_cast<ConstantInt>(C)) {
  if (!ConstantIsOk(CI))
    return llvm::None;
5144–5146 ↗(On Diff #221762)

Comment needs updating - the scalar case is no longer guaranteed to be handled,
so we can't talk about in the vector case.

This revision is now accepted and ready to land.Sep 25 2019, 7:29 AM
bjope added inline comments.Sep 25 2019, 8:39 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
5144–5146 ↗(On Diff #221762)

Maybe I've misunderstood something.

I've assumed that the ConstantIsOk check verified that the edge case had been handled (for the scalar case). And if not we bail out.
So in order to reach the code below this if-elseif-else, for scalars, the edge case should have been handled. Isn't that still true?

I can of course remove this TODO if you think that it isn't valid any longer.

lebedev.ri added inline comments.Sep 25 2019, 9:23 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
5144–5146 ↗(On Diff #221762)
// TODO? If the edge cases for vectors were guaranteed to be handled as they
// are for scalar

implies that we'll never get tautological inputs for scalars.
The old code asserted for that.
But now you remove that assert, thus saying there is no such guarantee scalars.
Therefore similarly we can't possibly ever have a similar guarantee for vectors.
At least that is how i parse all that after a quick look.

bjope updated this revision to Diff 221900.Sep 26 2019, 2:04 AM

I don't think we need to talk about the "edge cases" at all any longer (after reading the comment from Roman once more, and analysing the code a little bit).

> This diff only updates the code comments.

lebedev.ri accepted this revision.Sep 26 2019, 2:12 AM

Still LG.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
5144–5146 ↗(On Diff #221762)

Please mark comments as done as appropriate

bjope marked 4 inline comments as done.Sep 26 2019, 2:46 AM
bjope marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.