[PATCH] D21719: Unroll restructure

[Evgeny Stupachenko via llvm-commits]

evstupac added a comment.

> > Fix potential overflow.

> 

> 

> Is it possible to compose a test for it?


That is regarding 32 bits multiplication:

  (LoopSize - BEInsns) * UP.Count

I don't think we should make a test here. However I think it is possible.

> > Use BEInsns instead of constant 2.

> 

> 

> Why do we need it? At least, we can make it const I think.


That was comment from hfinkel in http://reviews.llvm.org/D19553.
BEInsns makes code easier to read. As for const value, yes it could be const now, but some architectures may have another value. Moreover loop analysis can point us to some other instruction that could be optimized after unroll. Tat way we'll be able to increase BEInsns.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:699
@@ +698,3 @@
+// and BEInsns.
+static bool isLoopSizeMeetUP(unsigned LoopSize,
+                             TargetTransformInfo::UnrollingPreferences &UP,
----------------
mzolotukhin wrote:
> s/isLoopSizeMeetUP/doesLoopSizeMeetUP/
> 
> Also, what about something like `isLoopSizeLessThanThreshold`?
>isLoopSizeLessThanThreshold
This function check if LoopSize less than threshold  but after unrolling. That way Count and BEInsns make differ.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:704-711
@@ -690,1 +703,10 @@
+  if (TT == Pragma)
+    return (uint64_t)(LoopSize - UP.BEInsns) * UP.Count +
+        UP.BEInsns <= PragmaUnrollThreshold;
+  else if (TT == Partial)
+    return (uint64_t)(LoopSize - UP.BEInsns) * UP.Count +
+        UP.BEInsns <= UP.PartialThreshold;
+  return (uint64_t)(LoopSize - UP.BEInsns) * UP.Count +
+      UP.BEInsns <= UP.Threshold;
+}
 // Returns true if unroll count was set explicitly.
----------------
mzolotukhin wrote:
> Why can't we just pass `PragmaUnrollThreshold`/`UP.PartialThreshold`/`UP.Threshold instead` of `ThresholdType TT`?
In most cases Threshold are in UP, so we need to pass only 2 parameters which is less. Also passing structure reference and its field value as different parameters looks strange.


Repository:
  rL LLVM

http://reviews.llvm.org/D21719