This is an archive of the discontinued LLVM Phabricator instance.

[mlir][arith] Add canonicalization patterns for 'mul*i_extended'
ClosedPublic

Authored by kuhar on Dec 10 2022, 1:46 PM.

Details

Summary
  • Add a fold for mulsi_extended(x, 1)
  • Add folds to demote wide integer multiplication to mul*i_extended when the result is shifted and truncated: trunci(shrui(mul(*ext(x), *ext(y)), c)) -> mul*i_extended(x, y)

Diff Detail

Event Timeline

kuhar created this revision.Dec 10 2022, 1:46 PM
kuhar requested review of this revision.Dec 10 2022, 1:46 PM
kuhar added inline comments.Dec 10 2022, 1:48 PM
mlir/lib/Dialect/Arith/IR/ArithCanonicalization.td
125

FYI, this is my first time writing non-trivial DDR, I'd appreciate feedback if you see how to simplify these or make them more idiomatic.

Can you update the description to explain the pattern? It's hard to tell at-a-glance what I'm looking for

Mogball accepted this revision.Dec 12 2022, 10:23 PM
Mogball added inline comments.
mlir/lib/Dialect/Arith/IR/ArithCanonicalization.td
125

This is as good as it gets

275

can you make sure these fit under 80 characters?

This revision is now accepted and ready to land.Dec 12 2022, 10:23 PM
kuhar updated this revision to Diff 482484.Dec 13 2022, 7:45 AM

Fix formatting in ArithCanonicalization.td. Rebase.

kuhar marked an inline comment as done.Dec 13 2022, 7:46 AM
kuhar edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Dec 13 2022, 7:51 AM
This revision was automatically updated to reflect the committed changes.
jpienaar added inline comments.
mlir/lib/Dialect/Arith/IR/ArithCanonicalization.td
289

Do you care about matching arith constantop explicitly? Else use constantlikematcher

mlir/lib/Dialect/Arith/IR/ArithOps.cpp
79

I would probably use getElementTypeOrSame followed by this rather than getElementTypeBitWidth.

85

With

(getScalarOrElementWidth($0) - getScalarOrElementWidth($1)) == "
        "getIntOrSplatIntValue($2)

it seems this will happily allow for known and unsupported cases with this chosen sentinel. And one could end up with $0 having width 1, $1 unsupported (so -1) and a shift amount of 2 and the constraint is happy.

100

-1 is a valid integer so using it as a sentinel here seems problematic given generic name of function.

kuhar added inline comments.Dec 13 2022, 8:12 AM
mlir/lib/Dialect/Arith/IR/ArithCanonicalization.td
289

These patterns don't but this seems to be the convention used in this file.

kuhar reopened this revision.Dec 13 2022, 11:25 AM
kuhar marked 3 inline comments as done.
kuhar added inline comments.
mlir/lib/Dialect/Arith/IR/ArithOps.cpp
85

I think this can't happen in valid IR because of the other type constraints

This revision is now accepted and ready to land.Dec 13 2022, 11:25 AM
kuhar updated this revision to Diff 482572.Dec 13 2022, 11:25 AM
kuhar marked an inline comment as done.

Address post-commit comments

kuhar updated this revision to Diff 482573.Dec 13 2022, 11:26 AM

Clean up includes

jpienaar accepted this revision.Dec 13 2022, 11:30 AM

Thanks!

This revision was landed with ongoing or failed builds.Dec 13 2022, 11:35 AM
This revision was automatically updated to reflect the committed changes.