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

[Andrey Turetskiy via llvm-commits]

aturetsk added a comment.

Hi Eric,
Thanks for the review.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1490
@@ +1489,3 @@
+
+unsigned X86TTIImpl::getLoopRotationDefaultThreshold() const {
+  // The loop rotation threshold equal to 2 shows the best performance and code
----------------
Added a comment.

================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:580
@@ -567,3 +579,3 @@
 class LoopRotate : public LoopPass {
-  unsigned MaxHeaderSize;
+  Optional<unsigned> SpecifiedThreshold;
 
----------------
Do I understand right that you don't like having Optional<> type for the variable which is not a function argument?

Unfortunately we can't use TTI object in the constructor, so we need to keep somehow the information whether the SpecifiedMaxHeaderSize was passed to the createLoopRotate or not until the chooseMaxHeaderSize is called. I see three ways to do that:
1) Use "Optional<unsigned> SpecifiedThreshold" as it is now
2) Use "int SpecifiedThreshold" and '-1' would mean the argument wasn't passed to the createLoopRotate. That's close to what the patch used to be.
3) Use 'unsigned SpecifiedThreshold' and 'bool IsSpecified'. The bool variable would indicate whether the argument was passed to the createLoopRotate or not.

Personally, I prefer the first option. BTW, I saw Optional<> used with class fields in other places (e.g. in lib/Transforms/Scalar/LoopUnrollPass.cpp in LoopUnroll class around the line 785).


http://reviews.llvm.org/D18898