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

[Z. Zheng via llvm-commits]

zzheng added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:580
@@ -567,3 +579,3 @@
 class LoopRotate : public LoopPass {
-  unsigned MaxHeaderSize;
+  Optional<unsigned> SpecifiedThreshold;
 
----------------
aturetsk wrote:
> 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).
I think either 1 or 3 will be fine. If we choose 3, we can remove the static unsigned chooseMaxHeaderSize().

```
class LoopRotate : public LoopPass {
unsigned SpecifiedThreshold;
bool IsSpecified;
...
LoopRotate(Optional<unsigned> SpecifiedMaxHeaderSize = None)
    : LoopPass(ID) {
  ...
  if (SpecifiedMaxHeaderSize.hasValue())
    SpecifiedThreshold = *SpecifiedMaxHeaderSize;
  else
    SpecifiedThreshold = RotationThreshold;

   IsSpecified = SpecifiedMaxHeaderSize.hasValue() || RotationThreshold.getNumOccurrences() > 0;
}
...
```

We can even change the if-else above to 
```
SpecifiedThreshold = SpecifiedMaxHeaderSize.hasValue() ?
                                *SpecifiedMaxHeaderSize : RotationThreshold;
```
as long as it conforms with the coding standard.

Later: 

```
    return iterativelyRotateLoop(
        L, IsSpecified ? SpecifiedThreshold : TTI->getLoopRotationDefaultThreshold(), LI, TTI, AC, DT, SE);
```


http://reviews.llvm.org/D18898