[PATCH] D104590: [LoopUnroll] Don't modify TripCount/TripMultiple in computeUnrollCount()

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:770
     TargetTransformInfo::UnrollingPreferences &UP,
     TargetTransformInfo::PeelingPreferences &PP, bool &UseUpperBound) {
 
----------------
nikic wrote:
> It would be nice to get rid of UseUpperBound as well, but apparently this is still being used by LoopUnrollAndJam in https://github.com/llvm/llvm-project/blob/876de062f94650f9ded56a22b062236f711fcd18/llvm/lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp#L177. No test coverage though :(
Ouch, this code is all around terrible isn't it?


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp:1166
   // Unroll factor (Count) must be less or equal to TripCount.
   if (TripCount && UP.Count > TripCount)
     UP.Count = TripCount;
----------------
I may be missing something, but I think you're changing behavior here.  computeUnrollCount appears to indirectly control ULO.Count via this line.  Unless I'm misreading this?  If I'm not, I'm surprised tests don't cover this.  


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

https://reviews.llvm.org/D104590