Page MenuHomePhabricator

[InstCombine] Replace one-use select operand based on condition
ClosedPublic

Authored by nikic on Jan 16 2021, 4:23 AM.

Details

Summary

InstCombine already performs a fold where X == Y ? f(X) : Z is transformed to X == Y ? f(Y) : Z if f(Y) simplifies. However, if f(X) only has one use, then we can always directly replace the use inside the instruction. To actually be profitable, limit it to the case where Y is a non-expr constant.

This could be further extended to replace uses further up a one-use instruction chain, but for now this only looks one level up.

Among other things, this also subsumes D94860.

Diff Detail

Event Timeline

nikic requested review of this revision.Jan 16 2021, 4:23 AM
nikic created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2021, 4:23 AM
aqjune accepted this revision.Jan 16 2021, 1:40 PM

LGTM

This revision is now accepted and ready to land.Jan 16 2021, 1:40 PM
This revision was landed with ongoing or failed builds.Jan 16 2021, 2:25 PM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Jan 18 2021, 1:07 PM

We're seeing crashes after this commit (5238e7b302ffc40707677960da9d64e872745dac) in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1167890

I don't have time to investigate more tonight, just wanted to give a heads up in case anyone else is also seeing problems here.

Hmm, would the pointer replacement be problematic here?
If there is a reduced version of a code, I think we can try

-    if (isa<Constant>(CmpRHS) && !isa<ConstantExpr>(CmpRHS))
+    if (isa<Constant>(CmpRHS) && !isa<ConstantExpr>(CmpRHS) && CmpRHS->getType()->isIntOrIntVectorTy())

and see whether the crash goes away.

tpopp added a subscriber: tpopp.Jan 18 2021, 2:45 PM

I've been unable to create a reproducer because of other optimizations getting in the way, but I'm seeing a case of infinite loops in inst combine due to both CmpLHS and CmpRHS being constants.

My issue at least is fixed by:

-    if (isa<Constant>(CmpRHS) && !isa<ConstantExpr>(CmpRHS))
+    auto IsConstant = [](auto *V) {return isa<Constant>(V) && !isa<ConstantExpr>(V);};
+    if (IsConstant(CmpRHS) && !IsConstant(CmpLHS))

That possible fix is here: https://reviews.llvm.org/D94934

My workday is over, so feel free to steal the solution or to offer a different one :) If this is a sign of a larger issue or leads to other problems, I'd propose we should roll this back as it's breaking existing code.

hans added a comment.Jan 19 2021, 2:54 AM

I found the file that's getting miscompiled, and uploaded preprocessed source and some notes here: https://bugs.chromium.org/p/chromium/issues/detail?id=1167890#c6

We do have code that matches what this change is looking for:

last_child_covers_ = child == nullptr ? previous_child : child;

And it seems that previous_child is getting folded to nullptr, which is incorrect. It's not clear whether it's this change in itself that's bad, or if it interacts badly with some underlying issue.

Hmm, would the pointer replacement be problematic here?
If there is a reduced version of a code, I think we can try

-    if (isa<Constant>(CmpRHS) && !isa<ConstantExpr>(CmpRHS))
+    if (isa<Constant>(CmpRHS) && !isa<ConstantExpr>(CmpRHS) && CmpRHS->getType()->isIntOrIntVectorTy())

and see whether the crash goes away.

Yes, it seems that it's a pointer that's being replaced, but I don't see why the value type should matter for this optimization.

I've reverted in 58bdfcfac048563e0dbcecc7c75e4e7897c8da18 until this can be figured out (that also reverted Tres's follow-up change).

Thanks for the reproducer @hans. I have applied a fixed version of this patch in 21443381c00d9d5ddd6a73f2f839dc4872d79463, which I believe should both fix the issue you saw, and a more general problem.

For the chromium issue, the problem is that we're replacing an operand of a phi node, but phi operands refer to values from their predecessor block, not the current block. As such an %x in a select and an %x in a phi operand may refer to different values and we can't replace them in the general case.

The more general problem is that we can only replace operands on speculatable instructions. If the instruction isn't speculatable, then replacing the operand might either cause an observable side-effect change, or cause UB.

Both of these issues are addressed by an additional isSafeToSpeculativelyExecute() check (phis are not considered speculatable).