[PATCH] D55710: add pragmas to control Software Pipelining optimisation

[Aaron Ballman via Phabricator via cfe-commits]

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