[PATCH] D97537: [Codegenprepare] Use IV increment instead of IV if we can prove it is not a poisoned value

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 4 09:52:19 PST 2021


reames added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:3874
+          ICmpInst::Predicate Pred;
+          if (match(BB->getTerminator(),
+                    m_Br(m_ICmp(Pred, m_Specific(IVInc), m_Value()),
----------------
aqjune wrote:
> nikic wrote:
> > spatel wrote:
> > > reames wrote:
> > > > Detail on the proof here -- the fact the branch is a loop exit is definitely irrelevant.
> > > > 
> > > > It's not clear to me whether branching on poison is immediate UB.  LangRef seems to say "yes", but the code in ValueTracking seems to say "no".  We need to draw in the experts here.  (Juneyoung)
> > > cc @aqjune @lebedev.ri @nikic for branch-on-poison question.
> > See D92739. Branch on poison is UB, but we're currently being conservative about it due to some known bugs (imho to our detriment).
> Hi,
> Yes, branching on poison is immediate UB, but it isn't listed in getGuaranteedWellDefinedOps (which is called by programUndefinedIfPoison). It's because there are a few known transformations that are poison-unsafe but still exist, and adding the case to getGuaranteedWellDefinedOps globally impacts optimizations, increasing opportunities for miscompilation bug.
> 
> The two most frequently happening (mis)transformations are short circuiting in SimplifyCFG (D95026) and select i1 -> and/or i1 in InstCombine (D93840). And then, there are a few optimizations that require either freeze or other workarounds, but they happen less frequently and fixing them is expected to have smaller performance impact.
> 
> For the SimplifyCFG patch: I believe it is almost there; combined with the enhancement in loop unswitch (which is mentioned in the patch) its impact is relatively small, as its asmdiff for llvm-testsuite result mentioned in its comments.
> It would be great if the patches could be reviewed and get in. :)
> 
> For the select -> and/or InstCombine opt: its fix is hidden under a flag, but I and people have already written many patches to resolve possible regressions (D93065 has links).
> 
@aqjune Given the proposed use case here is CGP, which is well after the specific mid-level optimizer cases you mention, what is your take on the practicality of using branching on poison as immediate UB here?  I'm tempted to say we should go ahead, but you're much more current with the practical status and tradeoffs than I am.  


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

https://reviews.llvm.org/D97537



More information about the llvm-commits mailing list