Page MenuHomePhabricator

Add loop unrolling metadata descriptions to LangRef.rst
Needs ReviewPublic

Authored by meheff on Jul 17 2014, 5:59 PM.

Details

Reviewers
hfinkel
Summary

This patch adds descriptions of the loop unrolling metadata optimization hints ("llvm.loop.unroll.*") to LangRefs.rst. The change also tweaks the language for the vectorization optimization hints to be consistent with the unrolling ones and the language in tools/clang/docs/LanguageExtension.rst.

Diff Detail

Event Timeline

meheff updated this revision to Diff 11621.Jul 17 2014, 5:59 PM
meheff retitled this revision from to Add loop unrolling metadata descriptions to LangRef.rst.
meheff updated this object.
meheff edited the test plan for this revision. (Show Details)
meheff added a reviewer: hfinkel.
meheff added a subscriber: Unknown Object (MLST).
hfinkel edited edge metadata.Jul 17 2014, 6:14 PM

Thanks for working on this!

docs/LangRef.rst
2913

You should add a note here that an IR-producer looking at affect this safety determination might find the llvm.mem.parallel_loop_access metadata useful.

2957

We should say something explaining how this is different from llvm.loop.vectorize.unroll (which actually does interleaving -- maybe we should rename it?). This is for concatenation unrolling (where the loop body is essentially replicated several times by the unroller, although other optimizations may intermix instructions from different unrolled iterations).

2965

I find this confusing. Specifically, to get partial unrolling, what needs to happen? If I want partial unrolling with a specific count, do I need both the 'count' and 'enable' metadata?

meheff added inline comments.Jul 18 2014, 10:25 AM
docs/LangRef.rst
2913

I'll add this.

2957

I think the fix here is to rename llvm.loop.vectorize.unroll to llvm.loop.vectorize.interleave. Then there is no chance of conflating the vectorize metadata with the unroll metadata. I'll work on a patch for this.

2965

I added a bit more explanation about that case.

I'd imagine an underlying cause of confusion is that `llvm.loop.unroll.enable 1` doesn't exactly mean enable. It means try to fully unroll the loop. So we want the following possible hints:

don't unroll
unroll fully
unroll by N

This doesn't exactly map nicely to the two metadata we have (one with boolean operand, one with i32). Maybe a cleaner way to have done this is with the following metadata:

llvm.loop.unroll.disable (no operand)
llvm.loop.unroll.fully_unroll (no operand)
llvm.loop.unroll.count i32

And only allow a single loop unroll metadata node per loop. Worth making this change?

  • Original Message -----

From: "Mark Heffernan" <meheff@google.com>
To: meheff@google.com, hfinkel@anl.gov
Cc: llvm-commits@cs.uiuc.edu
Sent: Friday, July 18, 2014 12:25:31 PM
Subject: Re: [PATCH] Add loop unrolling metadata descriptions to LangRef.rst

================
Comment at: docs/LangRef.rst:2913
@@ +2912,3 @@
+vectorizer will only vectorize loops if it believes it is valid to
do
+so.
+


hfinkel@anl.gov wrote:
> You should add a note here that an IR-producer looking at affect
> this safety determination might find the
> llvm.mem.parallel_loop_access metadata useful.
I'll add this.

================
Comment at: docs/LangRef.rst:2957
@@ +2956,3 @@
+optimizer believes it is valid to do so.
+
+'`llvm.loop.unroll.enable`' Metadata


hfinkel@anl.gov wrote:
> We should say something explaining how this is different from
> llvm.loop.vectorize.unroll (which actually does interleaving --
> maybe we should rename it?). This is for concatenation unrolling
> (where the loop body is essentially replicated several times by
> the unroller, although other optimizations may intermix
> instructions from different unrolled iterations).
I think the fix here is to rename llvm.loop.vectorize.unroll to

llvm.loop.vectorize.interleave.  Then there is no chance of

conflating the vectorize metadata with the unroll metadata. I'll
work on a patch for this.

Great, thanks!

================
Comment at: docs/LangRef.rst:2965
@@ +2964,3 @@
+bit operand value is 0 loop unrolling is disabled. A value of 1
+indicates that the loop should be fully unrolled. For example:
+


hfinkel@anl.gov wrote:
> I find this confusing. Specifically, to get partial unrolling, what
> needs to happen? If I want partial unrolling with a specific
> count, do I need both the 'count' and 'enable' metadata?
I added a bit more explanation about that case.

I'd imagine an underlying cause of confusion is that
`llvm.loop.unroll.enable 1` doesn't exactly mean enable. It means
try to fully unroll the loop. So we want the following possible
hints:

don't unroll
unroll fully
unroll by N

This doesn't exactly map nicely to the two metadata we have (one with
boolean operand, one with i32). Maybe a cleaner way to have done
this is with the following metadata:

llvm.loop.unroll.disable (no operand)
llvm.loop.unroll.fully_unroll (no operand)
llvm.loop.unroll.count i32

And only allow a single loop unroll metadata node per loop. Worth
making this change?

Yes, I think so. Maybe we can even do it before we branch for the release :-)

-Hal

http://reviews.llvm.org/D4576

meheff added inline comments.Jul 18 2014, 10:28 AM
docs/LangRef.rst
2965

I should add that this confusion is also baked into the pragma syntax. The following means unroll fully:

#pragma clang loop unroll(enable)