aaron.ballman added inline comments.
================ Comment at: include/clang/Basic/Attr.td:2871-2872 /// distribute: attempt to distribute loop if State == Enable + /// pipeline: disable pipelining loop if State == Disable + /// pipeline_ii_count: try to pipeline loop for only 'Value' value ---------------- alexey.lapshin wrote: > aaron.ballman wrote: > > Missing full stops at the end of the comments. Also, having read "for only > > 'Value' value", I'm still not certain what's happening. I think this is a > > count of some kind, so perhaps "Tries to pipeline 'Values' times." but I > > don't know what the verb "pipeline" means in this context. > > > > Are users going to understand what the `ii` means in the user-facing name? > As to ii - yes that should be understood by users, because it is important > property of software pipelining optimization. Software Pipelining > optimization tries to reorder instructions of original loop(from different > iterations) so that resulting loop body takes less cycles. It started from > some minimal value of ii and stopped with some maximal value. i.e. it tries > to built schedule for min_ii, then min_ii+1, ... until schedule is found or > until max_ii reached. If resulting value of ii already known then instead of > searching in range min_ii, max_ii - it is possible to create schedule for > already known ii. > > probably following spelling would be better : > > pipeline_ii_count: create loop schedule with initiation interval equals > 'Value' > because it is important property of software pipelining optimization. My point is that I have no idea what "ii" means and I doubt I'll be alone -- does the "ii" differentiate this from other kinds of pipeline loop pragmas we plan to support, or is expected that "pipeline_ii_count" be the only pipeline count option? Can we drop the "ii" and not lose anything? > pipeline_ii_count: create loop schedule with initiation interval equals > 'Value' equals 'Value' -> equal to 'Value' ================ Comment at: include/clang/Basic/DiagnosticParseKinds.td:1192 +def err_pragma_pipeline_invalid_keyword : Error< + "invalid argument; expected 'disable'">; ---------------- alexey.lapshin wrote: > aaron.ballman wrote: > > Can you roll this into the above diagnostic with another `%select`, or does > > that make it too unreadable? > yes, it makes things too complicated. Though I could do it if necessary. Not required, but I also didn't think the duplication here and below was a good approach either. But yeah, I'm not certain there's a better way to do this even if the list were rearranged. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55710/new/ https://reviews.llvm.org/D55710 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
