[PATCH] Add loop unroll pragma support

[Mark Heffernan]

Thanks for the suggestions.  They have been addressed.  Pekka (cc'd) on the optimizer patch thread suggested changing the metadata string from  llvm.loopunroll.* to llvm.loop.unroll.*.  I like the suggestion as it partitions the namespace more logically.  It makes sense then similarly to change the existing  llvm.vectorizer.* and llvm.interleave.* names to llvm.loop.vectorizer.* and llvm.loop.interleave.*.  Anybody have an opinion?

If I do make this change, then there will be a mismatch between the emitted metadata names for the vectorizer and what the optimizer expects.  What's the protocol for handling this?  Is submitting the optimizer change immediately after this clang change good enough?

================
Comment at: lib/Sema/SemaStmtAttr.cpp:126
@@ +125,3 @@
+  // Accumulated state of enable|disable hints for each hint category.
+  bool EnabledIsSet[3] = {false, false, false};
+  int EnabledValue[3];
----------------
Mark Heffernan wrote:
> Eli Bendersky wrote:
> > Reid Kleckner wrote:
> > > Rather than having 6 parallel arrays, maybe this would be better as an array of a struct:
> > >   struct {
> > >     int EnableOptionId;
> > >     int NumericOptionId;
> > >     bool EnabledIsSet;
> > >     bool ValueIsSet;
> > >     bool Enabled;
> > >     int Value;
> > >   } Options[] = {
> > >     {LoopHintAttr::Vectorize, LoopHintAttr::VectorizeWidth},
> > >     {LoopHintAttr::Interleave, LoopHintAttr::InterleaveCount},
> > >     {LoopHintAttr::Unroll, LoopHintAttr::UnrollCount}
> > >   };
> > The [size] on these arrays is superfluous? Also, Reid's suggestion makes sense if it ends up simplifying the code
> Good idea.  Done.
Removed.

================
Comment at: lib/Sema/SemaStmtAttr.cpp:126-136
@@ -117,1 +125,13 @@
+  // Accumulated state of enable|disable hints for each hint category.
+  bool EnabledIsSet[3] = {false, false, false};
+  int EnabledValue[3];
+  // Accumulated state of numeric hints for each hint category.
+  bool NumericValueIsSet[3] = {false, false, false};
+  int NumericValue[3];
 
+  int EnableOptionId[3] = {LoopHintAttr::Vectorize, LoopHintAttr::Interleave,
+                           LoopHintAttr::Unroll};
+  int NumericOptionId[3] = {LoopHintAttr::VectorizeWidth,
+                            LoopHintAttr::InterleaveCount,
+                            LoopHintAttr::UnrollCount};
+
----------------
Eli Bendersky wrote:
> Reid Kleckner wrote:
> > Rather than having 6 parallel arrays, maybe this would be better as an array of a struct:
> >   struct {
> >     int EnableOptionId;
> >     int NumericOptionId;
> >     bool EnabledIsSet;
> >     bool ValueIsSet;
> >     bool Enabled;
> >     int Value;
> >   } Options[] = {
> >     {LoopHintAttr::Vectorize, LoopHintAttr::VectorizeWidth},
> >     {LoopHintAttr::Interleave, LoopHintAttr::InterleaveCount},
> >     {LoopHintAttr::Unroll, LoopHintAttr::UnrollCount}
> >   };
> The [size] on these arrays is superfluous? Also, Reid's suggestion makes sense if it ends up simplifying the code
Good idea.  Done.

================
Comment at: lib/Sema/SemaStmtAttr.cpp:148
@@ -128,2 +147,3 @@
 
+    int Category;
     switch (Option) {
----------------
Eli Bendersky wrote:
> Would a local enum {Vectorize = 0, ...} type would make this code cleaner instead of magic Category numbers?
That's what I originally had when I wrote it, but the value is only used once to index into the Options array.  Since all categories are treated identically there is no code like, say, 'if (Category == Unroll)...' where the enum would add value.  As is the enum seems superfluous to me.

================
Comment at: lib/Sema/SemaStmtAttr.cpp:196
@@ -191,1 +195,3 @@
       }
+      if (EnabledIsSet[Category] && EnabledValue[Category] == 0) {
+        // Disable hints are not compatible with numeric hints of the
----------------
Eli Bendersky wrote:
> Just wondering: could this duplication be avoided if the check was separate and not part of setting hte attributes?
Done.  It also required changing a couple tests as it ends up swapping the pragma options in the error string.

http://reviews.llvm.org/D4089