[PATCH] D18898: [Loop Rotation] Make default max rotation header size threshold dependent on target CPU

[Hal Finkel via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:298
 
+  /// \brief Get target-customized default threshold for loop rotation.
+  unsigned getLoopRotationDefaultThreshold() const;
----------------
Please explain here what this threshold actually does (I imagine that this is the maximum header size). Unfortunately, LoopRotate barely documents what it does, so we might need to improve that in order for this to make sense.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1591
+
+unsigned X86TTIImpl::getLoopRotationDefaultThreshold() const {
+  // The loop rotation threshold equal to 2 shows the best performance and code
----------------
aturetsk wrote:
> echristo wrote:
> > Can you elaborate? What did you test on, do you know why? Is it just magic or is it based on something about the cpu?
> The positive effect was measured on Spec2000 (code size only, Spec2000 is too large to run on MCU) and Dhrystone (code size and performance). Also tested code size on ULP and code size with performance on Coremark, but there was no effect.
> The initial idea was to just improve code size for -Os by reducing the threshold, but since performance on Dhrystone improved as well (both for -Os and -O2), it seems to be a good idea to use a smaller threshold for Lakemont regardless of -O option.
> So this is based on mere tuning, no special idea behind it.
If this is something that is useful for a target to tune on a per-model basis, then we should add it to the scheduling model (for example, see LoopMicroOpBufferSize in SchedMachineModel and how that's handled).

We really should understand what is going on, however. Loop rotation uses CodeMetrics to calculate the effective size, but that cost model (TTI.getUserCost) has received very little attention. Also, does lakemont need any kind of scheduling model? It does not seem to have one at all.



https://reviews.llvm.org/D18898