[PATCH] D18202: Enable non-power-of-2 pragma unroll counts
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 17 12:46:57 PDT 2016
mzolotukhin accepted this revision.
mzolotukhin added a comment.
This revision is now accepted and ready to land.
This patch LGTM, thanks for working on this!
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:736-739
@@ -740,2 +735,6 @@
+ // If loop has an unroll count pragma mark loop as unrolled to prevent
+ // unrolling beyond that requested by the pragma.
+ if (HasPragma && PragmaCount != 0)
But maybe it's not a bad idea to have a 'nounroll' attribute on a loop that we cannot unroll:) Otherwise, next time we'll just probably spend some time trying again.
But I can see arguments for additional attempts to unroll when we have pragma, so if you feel strong about it, please keep it.
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:354-355
@@ +353,4 @@
+ // OR
+ // 2. The addition computing TripCount overflowed.
+ // If (2) is true, we know that TripCount really is (1 << BEWidth) and so
Oh, makes sense. Phabricator doesn't show whitespace changes, so I missed the change in indentation.
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:365-367
@@ +364,5 @@
+ Value *ModValAdd = B.CreateAdd(ModValTmp,
+ ConstantInt::get(ModValTmp->getType(), 1));
+ // At that point (BECount % Count) + 1 could be equal to Count.
> Yes. That is what I mean by select. I would prefer combiner to decide the correct replacement (maybe based on some architecture properties). The simplification of TripCount % Count to TripCount & (Count - 1) when Count is power-of-2 is pretty obvious and architecture independent. This one is more complicated: ((BECount % Count) + 1) % Count.
> So I'd let combiner do this if someone find a performance opportunity.
Thanks for the explanation, it's clear now. I think both `BECount == -1 ? <constant expr> : ((BECount +nuw 1) % Count` and `((BECount % Count) + 1) % Count` are good options here.
More information about the llvm-commits