[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
 
----------------
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?


================
Comment at: include/clang/Basic/AttrDocs.td:2583
+interleaving, and unrolling to be enabled or disabled. Pipelining could be disabled.
+Vector width as well as interleave, unrolling count and Initiation interval for pipelining
+can be manually specified. See `language extensions
----------------
Vector width, interleave count, unrolling count, and the initiation interval for pipelining can be explicitly specified.


================
Comment at: include/clang/Basic/AttrDocs.td:2654
+Specifying ``#pragma clang loop pipeline(disable)`` avoids software pipelining optimization.
+Only `disable` state could be specified for ``#pragma clang loop pipeline``:
+
----------------
The `disable` state can only be specified for ``#pragma clang loop pipeline``.


================
Comment at: include/clang/Basic/AttrDocs.td:2663
+
+Specifying the ii count parameter for ``#pragma loop pipeline_ii_count`` instructs software
+pipeliner to use only specified initiation interval :
----------------
Spell out `ii`

instructs software -> instructs the software


================
Comment at: include/clang/Basic/AttrDocs.td:2664
+Specifying the ii count parameter for ``#pragma loop pipeline_ii_count`` instructs software
+pipeliner to use only specified initiation interval :
+
----------------
to use only specified -> to use the specified

Remove the space before the colon as well.

Having read the docs, I would have no idea how to pick a value for the initiation interval. I'm still not even certain what it controls or does. Can you expand the documentation for that (feel free to link to other sources if there are authoritative places we can point the user to)?


================
Comment at: include/clang/Basic/AttrDocs.td:2676
+<http://clang.llvm.org/docs/LanguageExtensions.html#extensions-for-loop-hint-optimizations>`_
+for further details including limitations of the pipeline hints.
+  }];
----------------
details including -> details, including


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1178
   "%select{invalid|missing}0 option%select{ %1|}0; expected vectorize, "
-  "vectorize_width, interleave, interleave_count, unroll, unroll_count, or distribute">;
+  "vectorize_width, interleave, interleave_count, unroll, unroll_count, pipeline, pipeline_ii_count or distribute">;
 
----------------
80-col


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1192
+def err_pragma_pipeline_invalid_keyword : Error<
+    "invalid argument; expected 'disable'">;
 
----------------
Can you roll this into the above diagnostic with another `%select`, or does that make it too unreadable?


================
Comment at: lib/CodeGen/CGLoopInfo.cpp:29
       Attrs.UnrollAndJamCount == 0 &&
+      Attrs.PipelineDisabled == false &&
+      Attrs.PipelineIICount == 0 &&
----------------
`!Attrs.PipelineDisabled`


================
Comment at: lib/CodeGen/CGLoopInfo.cpp:127
 
+  if(Attrs.PipelineDisabled){
+      Metadata *Vals[] = {MDString::get(Ctx, "llvm.loop.pipeline.disable"),
----------------
Formatting is incorrect here (and below) -- you should run the patch through clang-format.


================
Comment at: lib/CodeGen/CGLoopInfo.h:70
+
+  /// Value for llvm.loop.pipeline.disable metadata
+  bool PipelineDisabled;
----------------
Missing full-stop. Here and elsewhere -- can you look at all the comments and make sure they're properly punctuated?


================
Comment at: lib/CodeGen/CGLoopInfo.h:71
+  /// Value for llvm.loop.pipeline.disable metadata
+  bool PipelineDisabled;
+
----------------
One good refactoring (don't feel obligated to do this) would be to use in-class initializers here.


================
Comment at: test/Parser/pragma-pipeline.cpp:24
+/* expected-error {{use of undeclared identifier 'error'}} */ #pragma clang loop pipeline_ii_count(error)
+/* expected-error {{expected '('}} */ #pragma clang loop pipeline_ii_count 1 2
+  for (int i = 0; i < Length; i++) {
----------------
Can you add a few tests:

`#pragma clang loop pipeline_ii_count(1` to show that we also diagnose the missing closing paren
`#pragma clang loop pipeline_ii_count(1.0)` to show that we diagnose a non-integer literal count
`#pragma clang loop pipeline enable` to show that we diagnose the missing open paren


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55710/new/

https://reviews.llvm.org/D55710