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

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

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

>> Should the setting be directly be applied in TTI::getCacheLineSize
>
> I would personally be in favor of such a change, however as I mentioned earlier the current default value of 0 might have been purposefully chosen to indicate a computer with no cache. For example I see this comment in MCSubtargetInfo.h:
>
>   /// Return the target cache line size in bytes.  By default, return
>   /// the line size for the bottom-most level of cache.  This provides
>   /// a more convenient interface for the common case where all cache
>   /// levels have the same line size.  Return zero if there is no
>   /// cache model.
>   ///
>   virtual unsigned getCacheLineSize() const {
>     Optional<unsigned> Size = getCacheLineSize(0);
>     if (Size)
>       return *Size;
>   
>     return 0;
>   }
>
> I don't think any of the targets supported run without some level of memory caching, but we need to run it by the wider community to make sure there are no surprises. Maybe a separate TTI hook for checking if a cache model exists would be the best compromise?

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 I did in this patch? To avoid breaking computers with no cache, I could update `CLS` in `LoopCacheCost.cpp` to the following :

  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.


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

https://reviews.llvm.org/D124926