This is an archive of the discontinued LLVM Phabricator instance.

[mlir][affine] Fix crash in AffineApplyOp canonicalization
ClosedPublic

Authored by springerm on Dec 5 2022, 7:45 AM.

Details

Summary

This test case used to crash with a failed assertion:

AffineExpr.cpp:659 in AffineExpr simplifyMul(AffineExpr, AffineExpr): lhs.isSymbolicOrConstant() || rhs.isSymbolicOrConstant()

This was caused by combining two affine maps, which created a multiplication of two non-symbols.

Diff Detail

Event Timeline

springerm created this revision.Dec 5 2022, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 7:45 AM
springerm requested review of this revision.Dec 5 2022, 7:45 AM
Herald added a project: Restricted Project. · View Herald Transcript
springerm updated this revision to Diff 480120.Dec 5 2022, 8:34 AM

fix invalid visitor usage

bondhugula requested changes to this revision.Dec 5 2022, 8:58 AM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1262–1263

Left over debug code.

mlir/test/Dialect/Affine/canonicalize.mlir
1220

I'm missing something. %0 isn't a valid symbolic operand here; it's a valid dimensional operand. So using %0 in the second affine.apply at ..[%0] is actually invalid, and the affine.apply op's verifier should fail. An isValidSymbol on %0 will return false. We shouldn't need a patch like this, and we shouldn't certainly need to determine whether a replacement will be invalid if the verification is in place?

This revision now requires changes to proceed.Dec 5 2022, 8:58 AM
springerm added inline comments.Dec 5 2022, 8:59 AM
mlir/test/Dialect/Affine/canonicalize.mlir
1220

Hmm so are you saying the input program is invalid? Is this something that should have been caught by the ApplyAffineOp verifier instead?

springerm added inline comments.Dec 5 2022, 9:01 AM
mlir/test/Dialect/Affine/canonicalize.mlir
1220

Follow-up question: Is this a valid input program?

func.func @regression_do_not_perform_invalid_replacements(%arg0: index) {
  %0 = affine.apply affine_map<(d0) -> (-d0 + 40961)>(%arg0)
  %1 = affine.apply affine_map<(d0)[s0] -> (d0 * (s0 ceildiv 512))>(%arg0)[%0]
  "test.foo"(%1) : (index) -> ()
  return
}
springerm added inline comments.Dec 5 2022, 9:11 AM
mlir/test/Dialect/Affine/canonicalize.mlir
1220

I'm asking because there is a canonicalization pattern that promotes the dim in %0 to a symbol. But I guess %1 would still be invalid and should not have been created in the first place?

bondhugula added inline comments.Dec 5 2022, 9:17 AM
mlir/test/Dialect/Affine/canonicalize.mlir
1220

In your snippet above, %0 is a valid symbol (it's defined at the top level). So using %0 in the second affine.apply is fine and that's valid IR. However, composing the two ops will yield a valid (semi-affine) map:

d0 * ((-s0 + 40961) ceildiv 512)
springerm added inline comments.Dec 5 2022, 9:25 AM
mlir/test/Dialect/Affine/canonicalize.mlir
1220

This second example can crash at the same assertion if the two canonicalization patterns are applied in the wrong order (i.e., promotion to symbol has to happen first, then compose the two affine maps, otherwise crash). The greedy pattern rewriter does not guarantee any order of pattern application.

springerm updated this revision to Diff 480368.Dec 6 2022, 12:58 AM
springerm marked an inline comment as done.

update test case

springerm updated this revision to Diff 480370.Dec 6 2022, 12:59 AM

remove unrelated changes

springerm added inline comments.Dec 6 2022, 1:02 AM
mlir/test/Dialect/Affine/canonicalize.mlir
1220

I updated the test case. Without this change, it crashes with -canonicalize="top-down=0". (In my use case, we don't use top-down=0, but this is the easiest way to reproduce it.)

bondhugula added inline comments.Dec 6 2022, 6:29 PM
mlir/test/Dialect/Affine/canonicalize.mlir
1220

Quick question: but promotion to symbol is not a pattern in itself - it's a utility/method. If I'm not mistaken, there is no pattern that just does promotion. It can always be applied before composition, and this invalid replacement can be avoided. Of course, one could still call replaceDimOrSym in a way that breaks its pre-requisite.

springerm updated this revision to Diff 480916.Dec 7 2022, 8:01 AM

canonicalize before replacement

springerm marked an inline comment as done.Dec 7 2022, 8:07 AM
springerm added inline comments.
mlir/test/Dialect/Affine/canonicalize.mlir
1220

That works. We can canonicalize the operand's map before performing any replacements.

A better static op verifier would still be nice. Will try to write one in a separate revision. Could be a pain, we'll see: I expect some breakages because there are likely some patterns that generate invalid affine.apply ops.

springerm edited the summary of this revision. (Show Details)Dec 7 2022, 8:08 AM
bondhugula added inline comments.Dec 8 2022, 10:31 AM
mlir/test/Dialect/Affine/canonicalize.mlir
1220

That's right - makes sense. That can be for another patch. Sorry about the delay here - I should be able to review/approve this tomorrow.

bondhugula accepted this revision.Dec 10 2022, 6:10 AM
This revision is now accepted and ready to land.Dec 10 2022, 6:10 AM
springerm closed this revision.Dec 12 2022, 12:13 AM
springerm marked an inline comment as done.