[PATCH] D86834: [InstSimplify] Protect against more poison in SimplifyWithOpReplaced (PR47322)

Sat Aug 29 08:32:18 PDT 2020

aqjune accepted this revision.
aqjune added a comment.
This revision is now accepted and ready to land.

The change looks good to me.

================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3799
     //       by dropping the flags. Remove this fold to save compile-time?
-    if (isa<OverflowingBinaryOperator>(B))
-      if (Q.IIQ.hasNoSignedWrap(B) || Q.IIQ.hasNoUnsignedWrap(B))
-        return nullptr;
-    if (isa<PossiblyExactOperator>(B) && Q.IIQ.isExact(B))
+    if (canCreatePoison(cast<Operator>(I)))
       return nullptr;
----------------
fhahn wrote:
> nikic wrote:
> > aqjune wrote:
> > > This diff has a side effect when IIQ's UseInstrInfo is false (and I think it is okay because this change leads to less potential poison-related problems, but) when is the flag set to false?
> > Yes, this is intentional. UseInstrInfo=false should result in less folds, not more. It may be even more correct to check for `|| !Q.IIQ.UseInstrInfo` here, i.e. make the conservative assumption that the instruction does have poison flags, if we are not allowed to inspect the actual poison flags on the instruction.
> > 
> > Maybe @fhahn can tell whether this is necessary.
> > when is the flag set to false?
> 
> It was added for NewGVN, which uses the `Simplify*` helpers with leader values from the corresponding congruence classes, but there is no guarantee that the final of the simplified instruction is dominated by those particular instructions which has this metadata.
Thanks for the explanation! :)
For people who's seeing this patch - a relevant bug is https://llvm.org/pr37540 .

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86834/new/

https://reviews.llvm.org/D86834