This is an archive of the discontinued LLVM Phabricator instance.

Use loop unrolling pragma metadata in the loop unroller
ClosedPublic

Authored by meheff on Jun 10 2014, 12:04 PM.

Details

Reviewers
eliben
Summary

I'm adding loop unrolling pragmas to clang with change (not yet submitted): http://reviews.llvm.org/D4089
This optimizer change consumes the metadata produced from those pragmas and uses them in the loop unroller.

The pragmas must immediately precede a loop statement and take the following forms:

#pragma clang loop unroll(enable) unroll the loop completely
#pragma clang loop unroll(disable)
do not unroll the loop.
#pragma clang loop unroll_count(N) // unroll the loop N times

These pragmas mirror recently added loop vectorizer pragmas. If you have comments on the particulars of the syntax and semantics please comment on the clang change.

Diff Detail

Event Timeline

meheff updated this revision to Diff 10292.Jun 10 2014, 12:04 PM
meheff retitled this revision from to Use loop unrolling pragma metadata in the loop unroller .
meheff updated this object.
meheff edited the test plan for this revision. (Show Details)
meheff added a reviewer: eliben.
meheff added a subscriber: Unknown Object (MLST).

Fyi, the proposed change to the LLVM optimizer is http://reviews.llvm.org/D4090

Great, this feature will be really useful. How about naming these metadatas using the llvm.loop. as a prefix? That is, llvm.loop.unroll.enable etc? Maybe it's worth adding a test case with a negative unroll count to ensure nothing bad happens. Otherwise LGTM.

eliben added inline comments.Jun 10 2014, 4:02 PM
lib/Transforms/Scalar/LoopUnrollPass.cpp
274

Move this closer to use

test/Transforms/LoopUnroll/unroll-pragmas.ll
49

IMHO it would be more readable if you placed !1 right after this function. I don't think the mdnodes have to be necessarily collected in the bottom. Ditto for the rest

reames added a subscriber: reames.Jun 11 2014, 10:25 AM

Just to check, the semantics of the pragmas are best effort right? They're not *required* to be respected for semantic correctness? If they were, metadata would be the wrong implementation mechanism here.

lib/Transforms/Scalar/LoopUnrollPass.cpp
181

Should this be a cast? Is it ever expected to fail?

190

I find the semantics of this function slightly confusing. It might make sense to separate two helper functions: one for the enable case and one for the disable case. They can share implementation if desired.

267

Can a user specified count be zero or negative? This seems like one way to disable loop unrolling.

This is not a new issue in the code you added, I just happened to notice it here.

288

Does it make sense to clamp this to something reasonable? Unrolling a million iteration loop seems like a spectacularly bad idea and probably not actually the users intent.

meheff updated this revision to Diff 10332.Jun 11 2014, 1:36 PM

Thanks for your suggestions. I think I've addressed all of them.

Great, this feature will be really useful. How about naming these metadatas using the llvm.loop. as a prefix? That is, llvm.loop.unroll.enable etc? Maybe it's worth adding a test case with a negative unroll count to ensure nothing bad happens. Otherwise LGTM.

I like the llvm.loop.unroll. prefix idea. As mentioned in the clang patch thread, it makes sense to change the loop vectorizer metadata names as well. I will do that in a followup patch.

I added an assert for the non-positive case because zero and negative unroll counts should never be emitted by clang (it emits an error). As mentioned in my inline comments nonpositive unroll factors are possible by using a test-only command line opt and via a pass constructor arg, but that doesn't seem to be a significant problem. Let me know if that's reasonable.

In D4090#8, @reames wrote:

Just to check, the semantics of the pragmas are best effort right? They're not *required* to be respected for semantic correctness? If they were, metadata would be the wrong implementation mechanism here.

They are just optimization hints and are not required for correctness.

lib/Transforms/Scalar/LoopUnrollPass.cpp
181

It should always be a ConstantInt. Changed to cast.

190

Looking at it again. It is confusing, indeed :-) Split into two functions.

267

Yeah, the code currently doesn't deal with user-specified values <= 0 well. The value of zero will essentially be ignored and the usual heuristics will apply. If the value is negative it gets converted to unsigned which means a very large unrolling factor. The cl opt is unsigned, so a negative value can only come from the constructor args. Overall probably not optimal, but this nonpositive value badness is only exercisable via a test-only cl opt and bad args to the pass constructor so probably not a big issue. Happy to fix it if you'd like though.

Regarding making values <=0 disable unrolling, there are other ways to disable unrolling (cl opts, programmatically, and with pragmas in the code) so it seems unnecessary to me.

274

Done.

288

Makes sense. Added a limit of 1024. I only chose to only enforce this limit for the unroll(enable) case because this would be the most surprising case to the user. If the user specified unroll_count(1000000), well, then they've knowingly shot themselves in the foot. Lemme know if you think that's reasonable. Also added a test for this case.

test/Transforms/LoopUnroll/unroll-pragmas.ll
49

Done.

I responded to all the comments via Phabricator over an hour ago, but
Phabricator never sent out the corresponding email :-( I've verified that
others can see my comment at http://reviews.llvm.org/D4090 so please look
there for my responses and latest patch.

Mark

With changes, seems fine to me.

(I do not commit rights, thus this is not an official LGTM.)

lib/Transforms/Scalar/LoopUnrollPass.cpp
58

a) I would choose a slightly higher value. The reason for the limit is mostly to avoid absolutely insane behaviour. i.e. 100k might be a good one
b) Does the frontend enforce any type of limit?
c) Might be worth making this an command line option

None of these are must change, use your judgement.

eliben accepted this revision.Jun 11 2014, 4:07 PM
eliben edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 11 2014, 4:07 PM
meheff closed this revision.Jun 12 2014, 2:53 PM
meheff added inline comments.Jun 14 2014, 3:40 PM
lib/Transforms/Scalar/LoopUnrollPass.cpp
58

(Looks like phabricator never sent out your comment at least to me, sigh)

Anyway, to answer your questions/comments. The frontend does not enforce a limit other than that the count has to be a positive value. The patch got reverted because it was a suspect in a build breakage. The second attempt includes making it a cl opt as you suggest and uses a size threshold rather than a count threshold. See http://reviews.llvm.org/D4147.