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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
