[PATCH] D37051: Model cache size and associativity in TargetTransformInfo

[Tobias Grosser via Phabricator via llvm-commits]

grosser marked 2 inline comments as done.
grosser added a comment.

Hi Alex, hi Roman,

thanks for your feedback. I addressed your comments.

Best,
Tobias



================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:607
+  /// The possible cache levels
+  enum CacheLevel {
+    CL_L1,   // The L1 cache
----------------
asb wrote:
> It's conceivable that targets might want to differentiate between the I-cache and D-cache. e.g an embedded target may use info on the I-cache to guide custom code layout optimisations.
> 
> Is it worth starting with CL_L1D and CL_L2D instead?
Very good point. I changed this.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:348
+    case TargetTransformInfo::CL_L2:
+      return 256 * 1024; // 128 KByte
+    }
----------------
asb wrote:
> Either the comment or the calculation is incorrect!
The calculation was wrong. I fixed this.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:92
+    //   - Kabylake
+    return 256 * 1024; // 128 KByte
+  }
----------------
asb wrote:
> Either the comment or the calculation is incorrect!
The calculation. Fixed.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.h:49
   /// @}
+  unsigned getCacheSize(TargetTransformInfo::CacheLevel Level) const;
 
----------------
asb wrote:
> Why not implement getCacheAssociativity too?
Added!


https://reviews.llvm.org/D37051