Page MenuHomePhabricator

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

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



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.


Diff Detail


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!

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

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.