[PATCH] D124926: [LoopInterchange] New cost model for loop interchange

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

In D124926#3571264 <https://reviews.llvm.org/D124926#3571264>, @bmahjour wrote:

>> Thanks for the comment, I see your concern. Would you like me to continue D127342 <https://reviews.llvm.org/D127342>, or would you like me to still keep the cache line size option under `LoopCacheCost.cpp` as we did in this patch? To avoid breaking computers with no cache, I could update `CLS` in `LoopCacheCost.cpp` to the following (I may need to change to wording a little bit) :
>>
>>   CLS = DefaultCacheLineSize.getNumOccurrences() > 0  ? DefaultCacheLineSize : TTI.getCacheLineSize()
>>
>> and pass -cache-line-size=64 to all loop interchange tests. This way the buildbot test failures can be avoided, and in the meantime for a realworld computer with no cache it won't break.
>
> I slightly prefer to continue with D127342 <https://reviews.llvm.org/D127342>, assuming you bring it up with the wider community (eg. through Discourse) and there are no objections. Not having cache is an exception (at best) rather than the norm, so it doesn't make much sense to me that the default assumes no cache. 
> If that doesn't pan out I'm ok with your second suggestion, but we'd also need to teach LoopCacheCost to handle CLS == 0 (otherwise it would divide by zero).

Thanks for the suggestions, I'll bring it up through Discourse and if there are no objections I'll continue with D127342 <https://reviews.llvm.org/D127342>.

Regarding "divide by zero" it seems to me that if `CLS==0` in LoopCacheCost then `isConsecutive()` will always be false, hence we will never calculate this equation `(TripCount*Stride)/CLS`. But anyways this seems a subtle point - I'll pursue D127342 <https://reviews.llvm.org/D127342> for now.


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

https://reviews.llvm.org/D124926