Page MenuHomePhabricator

[mlir][Shape] Generalize cstr_broadcastable folding for n-ary broadcasts
ClosedPublic

Authored by bkramer on Feb 16 2021, 10:12 AM.

Details

Summary

This is still fairly tricky code, but I tried to untangle it a bit.

Diff Detail

Event Timeline

bkramer created this revision.Feb 16 2021, 10:12 AM
bkramer requested review of this revision.Feb 16 2021, 10:12 AM
bkramer updated this revision to Diff 324047.Feb 16 2021, 10:17 AM

Fix OOB condition

tpopp accepted this revision.Feb 17 2021, 1:48 AM
tpopp added inline comments.
mlir/lib/Dialect/Shape/IR/Shape.cpp
505

I'm not sure if this optimization is really worth it? You could also make this return true here and change the name to hasAtMostSingleNonScalar if you want though.

mlir/lib/Dialect/Traits.cpp
25

Can you not have nested ArrayRef's?

26

Maybe add an assert for shapes.size()

31

Random thought: It would be nice to use zip_longest somehow for these loops, but it seems not feasible.

33

Ideally a better name like nonOneDim (not that that's great either) or a comment indicating what this will be used for.

41

super nit: stray '.'

41

super nit 2: "rest are 1"

This revision is now accepted and ready to land.Feb 17 2021, 1:48 AM
bkramer updated this revision to Diff 324251.Feb 17 2021, 2:43 AM
bkramer marked 7 inline comments as done.

Address review comments

mlir/lib/Dialect/Traits.cpp
25

The only way I know of is creating an extra SmallVector<ArrayRef<>> on the caller side. I felt like that's worse than just requiring an ArrayRef of SmallVectors.

31

zip can only do a static number of elements.

This revision was landed with ongoing or failed builds.Feb 17 2021, 2:53 AM
This revision was automatically updated to reflect the committed changes.