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

[Nikita Popov via Phabricator via llvm-commits]

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

LGTM



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6598
+    return &*F.getEntryBlock().begin();
+  }
   return nullptr;
----------------
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.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6607
     return isGuaranteedToTransferExecutionToSuccessor(&I);
   });
 }
----------------
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.


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

https://reviews.llvm.org/D109845