Page MenuHomePhabricator

[InstSimplify] do not propagate poison from select arm to icmp user
ClosedPublic

Authored by spatel on Jul 1 2021, 10:50 AM.

Details

Summary

This appears to be the root cause of the miscompile in:
https://llvm.org/PR50944
...and possibly other recently filed bugs.

The problem has likely existed for some time, but it was made visible with:
5af8bacc94024 ( D104661 )
handleOtherCmpSelSimplifications() assumes it can convert select of constants to bool logic ops, and that does not work with poison.

The bug is in instsimplify, but I'm not sure how to reproduce it outside of instcombine. The reason this is visible in instcombine is because we have a hack (FIXME) to bypass simplification of a select when it has an icmp user:
https://github.com/llvm/llvm-project/blob/955f12589940634acc6c9901e8b25534808f691c/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp#L2632

So we get to an unusual case where we are trying to simplify an instruction that has an operand that would have already simplified if we had processed it in normal order.

Diff Detail

Event Timeline

spatel created this revision.Jul 1 2021, 10:50 AM
spatel requested review of this revision.Jul 1 2021, 10:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2021, 10:50 AM

I'm skeptical it's correct to explicitly check for PoisonValue here. Does it matter if the value is poison, but not a poison literal? Does it matter if a constant vector has a poison element?

I'm skeptical it's correct to explicitly check for PoisonValue here. Does it matter if the value is poison, but not a poison literal? Does it matter if a constant vector has a poison element?

Yes, good point. We want to use isGuaranteedNotToBePoison() to be safe IIUC.
I've been trying to find a hole through other folds that would show that difference, but I haven't found one yet...

nikic added inline comments.Jul 1 2021, 12:11 PM
llvm/lib/Analysis/InstructionSimplify.cpp
508

I think the right place to address this is in handleOtherCmpSelSimplifications. It basically performs the select i1 %a, i1 %b, i1 false -> and i1 %a, %b fold that we have recently disabled in InstCombine. We should guard it by the same impliesPoison() check.

spatel updated this revision to Diff 356001.Jul 1 2021, 1:24 PM
spatel marked an inline comment as done.

Patch updated:
Use impliesPoison() to match the similar code in InstCombine.

nikic accepted this revision.Jul 1 2021, 1:28 PM

LGTM

This revision is now accepted and ready to land.Jul 1 2021, 1:28 PM
This revision was landed with ongoing or failed builds.Jul 1 2021, 2:47 PM
This revision was automatically updated to reflect the committed changes.