[PATCH] D109845: [SCEV] Correctly propagate nowrap flags across scopes when folding invariant add through addrec

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6598
+    return &*F.getEntryBlock().begin();
+  }
   return nullptr;
----------------
nikic wrote:
> reames wrote:
> > reames wrote:
> > > nikic wrote:
> > > > Might make sense to just make `&*F.getEntryBlock().begin()` the fallback return value? That one should hold for all SCEVs, not just SCEVConstant in particular.
> > > Hm, with the current code structure, you're right.  Once I build the recursion interface, I'll want to go back to the nullptr form (as a signal to recurse), but this is a good idea.  
> > Ran a test with this, and don't really see much meaningful test diff.  Still worth doing, but do you mind if I leave it to a follow on patch?  
> I'm fine with that, but also don't really see the point of landing it separately. The suggestion was mostly intended as code cleanup, as I found the current treatment of SCEVConstant unintuitive.
Addressed in 35ab21.  As you'll note, I needed to rename a bit and restructure the code to make the bounding explicit to make it clear to me.  :)


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6607
     return isGuaranteedToTransferExecutionToSuccessor(&I);
   });
 }
----------------
nikic wrote:
> These instruction walks have pathological cases for large BBs. We should limit this walk to a small constant number of instructions. E.g. the `programUndefinedIfPoison()` check looks through 32 guaranteed-to-execute instructions. (This is fine as a followup.)
> 
> Alternatively we could add an ICF-style cache for this, but I am somewhat hesitant to suggest adding yet another cache that requires invalidation to SCEV.
Addressed in 5f7a53.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109845