On Tue, May 13, 2014 at 3:22 PM, Tyler Nowicki <[email protected]> wrote: > Hi Aaron, Richard, > >> After committing r208702/3, your tests now pass for me. I'll take it >> on faith that the comment and formatting gets resolved when you >> commit, so LGTM. You should wait for Richard to sign off as well, >> since he had some comments. > > > Thanks for all the reviewing, I’m glad this is getting closer to being > committed! > > @Richard, please take a look at the attached patch. > > >> As it's written, it implies that you must specify vectorize or >> interleave, followed by disable/enable/value, and then optionally >> supply a value. Eg) #pragma loop vectorize(enable, 4) >> >> That's why I think we should have a Union argument type because you >> really want it to either be enable|disable, or an integer value, not >> both. >> >> Not something that needs doing for this patch by any means. :-) > > I’ll do that as a follow-up.
I have ideas on how it should be implemented, so it's on my radar as well. > > >>> I'd be nice to have a command-line switch to turn on and off this pragma. >>> For example skipping "-fopenmp" makes a compiler to ignore OMP pragmas. >>> So if you add this switch one can analyse the pure effect of using >>> vectorizing pragmas at the specified locations. >> >> Couldn't that be accomplished via macros already, where you have a >> macro definition for #pragma loop, and simply noop the macro from the >> command line with -D? > > > LLVM doesn’t seem to allow you to define a pragma in a macro. It seems to > expect the macro to only contain expressions. This is probably a bug? You have to use _Pragma to accomplish it (C99, IIRC). >> When you reintroduce your changes, I think I would prefer to leave the >> iteration in its current (forward) order instead of reverse order (as >> you have it in your patch). That's simply a bug, and working around it >> here is likely to ensure that bug sticks around even longer. Once you >> have introduced your changes, I'd appreciate an extra test case which >> uses -ast-print and demonstrates the reversed order (I am okay with us >> XFAILing that test) so that gives us a target test to use to verify >> the fix when it does happen. > > I’ll add the test case in a separate patch since it isn’t related to this > work. I’m not really thrilled about the compiler producing pragmas in the > reverse order in the mean time though. I updated the patch and tests so they > pass with the forward iteration (reverse order) attributes. Thanks! It's on my radar and I hope to get to it sooner rather than later. ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
