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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert reopened this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

Again, glancing at these transformations makes me doubt how well they are tested. The documentation doesn't seem to indicate this is a WIP or not ready yet.
I only read over parts and found 3 things that I suspect are "broken", for some definition of the word. (The multi symbol stuff is technically not a correctness issue.)

I'm also guessing we ignore overflows and such completely when we do these things, right?



================
Comment at: flang/lib/Optimizer/Transforms/AffinePromotion.cpp:204
+    if (auto op = value.getDefiningOp<mlir::MulIOp>())
+      return affineBinaryOp(mlir::AffineExprKind::Mul, op.lhs(), op.rhs());
+    if (auto op = value.getDefiningOp<mlir::UnsignedRemIOp>())
----------------
This way of translating to "affine" should break as soon as lhs or rhs is an induction variable and the other one is not a constant.
Same for other binary operations. I didn't see a test for this but I would hope something in affine will complain.


================
Comment at: flang/lib/Optimizer/Transforms/AffinePromotion.cpp:215
+        return {mlir::getAffineDimExpr(dimCount++, value.getContext())};
+      return {mlir::getAffineSymbolExpr(symCount++, value.getContext())};
+    }
----------------
This assumes perfectly nested loops, right? Is this checked?
I doubt the symCount stuff works if a symbol is used multiple times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111155