[PATCH] D77870: [MLIR] Introduce utility to hoist affine if/else conditions

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Affine/Utils.h:24
+/// hoisting could lead to significant code expansion in some cases.
+bool hoistAffineIfOp(AffineIfOp ifOp, bool *folded = nullptr);
+
----------------
Why not LogicalResult?


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:66
+
+// Returns true if the hoisting happened or if the op was folded away.
+bool mlir::hoistAffineIfOp(AffineIfOp ifOp, bool *folded) {
----------------
Please use ///


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:118
+  // version.
+  StringRef idForIfOp = "__mlir_if_hoisting";
+  operandMap.clear();
----------------
Cache the Identifier to avoid string comparison in attribute lookup.


================
Comment at: mlir/lib/Dialect/Affine/Utils/Utils.cpp:159
+  applyPatternsAndFoldGreedily(
+      hoistedIfOp.getParentWithTrait<OpTrait::IsIsolatedFromAbove>(),
+      std::move(patterns));
----------------
I would have expected the if you want to remove dead else blocks you could specifically target the ops you know about, instead of running on the entire function.


================
Comment at: mlir/test/lib/Dialect/Affine/TestIfHoist.cpp:28
+  TestIfHoist() = default;
+  TestIfHoist(const TestIfHoist &pass){};
+
----------------
Remove the semicolon


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77870