[PATCH] D114784: [SCEV] Track backedge taken count users (NFCI)

[Philip Reames via Phabricator via llvm-commits]

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

Thanks for explaining.

LGTM w/suggested, but not required changes.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7891
          "No point in having a non-constant max backedge taken count!");
-
-  SCEVRecordOperands RecordOperands(Operands);
----------------
nikic wrote:
> reames wrote:
> > Basic idea makes sense, but I really think the user tracking should be here.  Much easier to tell for sure we caught everything.
> > 
> > For one thing, you don't appear to be tracking the constantmax information in your patch, and I think we do need to add that.
> > Basic idea makes sense, but I really think the user tracking should be here. Much easier to tell for sure we caught everything.
> 
> Generally agree -- reason I didn't put it in here is that the user tracking is on ScalarEvolution, so we'd have to pass it in and make BackedgeTakenInfo a friend class. The resulting layering seems a bit odd.
> 
> > For one thing, you don't appear to be tracking the constantmax information in your patch, and I think we do need to add that.
> 
> I intentionally dropped the ConstantMax part, because it always stores a SCEVConstant, which can never be invalidated (for ExactNotTaken, only non-SCEVConstants are tracked as well). We don't track ENT.MaxNotTaken (both before and after this patch) for the same reason, it's always constant.
> 
> 
On the placement, maybe move it just before the return which constructs the BackedgeTakenInfo?  Or at least add an assert there to make sure we covered all the exit cases?  I'm worried about someone complicating the flow through that method and forgetting to update the users.

Oh!  That reminds me, we should really be verifying this.  (i.e, making sure our user sets are in sync with the forward direction)  I'm fine having that in a separate patch, but can you make sure that happens?

On the ConstantMax case, makes sense.  Can you make sure to add a comment somewhere emphasizing the subtlety?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114784