[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/AttrDocs.td:2582
 specified for the subsequent loop. The directive allows vectorization,
-interleaving, and unrolling to be enabled or disabled. Vector width as well
-as interleave and unrolling count can be manually specified. See
-`language extensions
+interleaving, and unrolling to be enabled or disabled. Pipelining could be disabled.
+Vector width, interleave count, unrolling count, and the initiation interval for pipelining
----------------
alexey.lapshin wrote:
> aaron.ballman wrote:
> > Can you combine the new sentence with the previous one? `The directive allows vectorization, interleaving, pipelining, and unrolling to be enabled or disabled.`
> In that case it would change a meaning and become incorrect. Pipelining could only be disabled. It could not be enabled.
Ah, yes, that would change the meaning then. I'd go with: `The directive allows pipelining to be disabled, or vectorization, interleaving, and unrolling to be enabled or disabled.`


================
Comment at: include/clang/Basic/AttrDocs.td:2663
+  could be used as hints for the software pipelining optimization. The pragma is
+  placed immediately before a for, while, do-while, or a C++11 range-based for
+  loop.
----------------
I'd like to see a Sema test where the pragma is put at global scope to see if the diagnostic is reasonable or not. e.g.,
```
#pragma clang loop pipeline(disable)

int main() {
  for (int i = 0; i < 10; ++i)
    ;
}
```


================
Comment at: include/clang/Basic/AttrDocs.td:2680-2681
+  the specified cycle count. If a schedule was not found then loop
+  remains unchanged. The initiation interval must be a positive number
+  greater than zero:
+
----------------
Please add a Sema test with a negative number as the initiation interval.


================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:1188
+  "vectorize_width, interleave, interleave_count, unroll, unroll_count, "
+  "pipeline, pipeline_initiation_interval or distribute">;
 
----------------
pipeline_initiation_interval or distribute -> pipeline_initiation_interval, or distribute

(Keeps the Oxford comma as in the original diagnostic.)


================
Comment at: test/Parser/pragma-pipeline.cpp:28
+/* expected-error {{invalid argument of type 'double'; expected an integer type}} */ #pragma clang loop pipeline_initiation_interval(1.0)
+/* expected-error {{invalid value '0'; must be positive}} */ #pragma clang loop pipeline_initiation_interval(0)
+  for (int i = 0; i < Length; i++) {
----------------
This isn't really a parser test -- it should probably be in Sema instead.


================
Comment at: test/Parser/pragma-pipeline.cpp:35-47
+#pragma clang loop pipeline(disable) 
+/* expected-error {{expected a for, while, or do-while loop to follow '#pragma clang loop'}} */ int j = Length;
+#pragma clang loop pipeline_initiation_interval(4)
+/* expected-error {{expected a for, while, or do-while loop to follow '#pragma clang loop'}} */ int k = Length;
+
+#pragma clang loop pipeline(disable)
+#pragma clang loop pipeline_initiation_interval(4) /* expected-error {{incompatible directives 'pipeline(disable)' and 'pipeline_initiation_interval(4)'}} */
----------------
These should also move to Sema.


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

https://reviews.llvm.org/D55710