[PATCH] D111155: [fir] Add affine promotion pass

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski accepted this revision.
awarzynski added a comment.

LGTM. Thanks for adding the tests!



================
Comment at: flang/test/Fir/affine-promotion.fir:8
+
+func @calc(%a1: !arr_d1, %a2: !arr_d1, %a3: !arr_d1) {
+  %c1 = constant 1 : index
----------------
[nit] Could you rename `calc` as `loop_with_load_and_store` or something similar? So that it is clear which case is being tested?


================
Comment at: flang/test/Fir/affine-promotion.fir:81
+
+func @calc(%a: !arr_d1, %v: f32) {
+  %c0 = constant 0 : index
----------------
[nit] Could you rename `calc` as `loop_with_if` or something similar? So that it is clear which case is being tested?


================
Comment at: flang/test/Fir/affine-promotion.fir:93
+        %cond = cmpi "sgt", %im2, %c0 : index
+        fir.if %cond {
+          %a_idx = fir.array_coor %a(%dims) %i
----------------
clementval wrote:
> awarzynski wrote:
> > Would a test with `fir.if` _outside of_ `fir.do_loop` be possible? I think that it would nicely highlight  _what_ is being tested. Currently both versions of `@calc` look like tests for `fir.do_loop` specifically (that's the first thing that you notice). `fir.if`/affine.if` or `fir.load`/`affine.store` are not obvious to spot.
> a `fir.if` outside of loop would never produce affine operations. `affine.if` has meaning in a loop only. 
Ah, indeed:

> The affine.if operation restricts execution to a subset of the loop iteration space defined by an integer set (a conjunction of affine constraints).

>From : https://mlir.llvm.org/docs/Dialects/Affine/#affineif-mliraffineifop. I missed that, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111155