Page MenuHomePhabricator

[mlir] scf::ForOp: Propagate constants in loop body to trigger simplifications
AbandonedPublic

Authored by chelini on Mar 31 2021, 1:56 AM.

Diff Detail

Event Timeline

chelini created this revision.Mar 31 2021, 1:56 AM
chelini requested review of this revision.Mar 31 2021, 1:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 1:56 AM
ftynse requested changes to this revision.Apr 6 2021, 2:50 AM

Would you mind providing a commit description in addition to the one-line summary? A good description explains _why_ this new functionality is useful, rather than what the change does.

mlir/lib/Dialect/SCF/SCF.cpp
719

Nit: could you put the results of all std::get<N>(it) into named variables? It would make the code more readable.

mlir/test/Dialect/SCF/canonicalize.mlir
523

Nit: could this match .* instead of the specific regexp?

526

I don't think it's important for this code to be on the next line, so let's just use CHECK and avoid over-constraining the tests.

536

It looks like this tests for a sequence of canonicalizations. Could we in addition have a test that exercises only the new canonicalization you are adding? This helps bisecting problems.

This revision now requires changes to proceed.Apr 6 2021, 2:50 AM
wsmoses added inline comments.Wed, Apr 21, 7:43 PM
mlir/lib/Dialect/SCF/SCF.cpp
720

Is there a reason why this has to be constant for this to apply. This looks like LICM to me and otherwise doesn't need the constantop req?

chelini abandoned this revision.Thu, Apr 22, 1:17 AM

This is a wrong approach in using the pattern rewrite see: https://mlir.llvm.org/docs/PatternRewriter/#restrictions. I will close this.