Page MenuHomePhabricator

[mlir] Prevent assertion failure in DropUnitDims

Authored by tpopp on Aug 17 2021, 6:33 AM.



Don't assert fail on strided memrefs when dropping unit dims.
Instead just leave them unchanged.

Diff Detail

Event Timeline

tpopp created this revision.Aug 17 2021, 6:33 AM
tpopp requested review of this revision.Aug 17 2021, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 6:33 AM
mravishankar requested changes to this revision.Aug 17 2021, 12:12 PM
mravishankar added inline comments.

This seems a bit hard to follow. Can we instead make this return an Optional<...> and handle the case where nothing changes at the call site?

This revision now requires changes to proceed.Aug 17 2021, 12:12 PM
tpopp added inline comments.Aug 18 2021, 12:32 AM

The caller takes these data structures and uses them at quite a few places and most of them need 1:1 mappings with the original input/outputs, so the alternative of returning an Optional would move this code to the caller. The caller would still need to generate this same information. There's no simple way to change that due to the caller needing the same number of data structures as the original operation had.

Given that this information needs to be generated regardless, I think it's better to just return it in here rather than have an additional if/else statement in the caller. This function can already return this NoOp like result also, so this doesn't result in a situation for the caller that didn't exist before.

tpopp marked an inline comment as done.Aug 19 2021, 7:42 AM
tpopp added a reviewer: gysit.Aug 23 2021, 8:11 AM
mravishankar added inline comments.Aug 23 2021, 9:32 AM

I find it hard to maintain/read code if there is a whole data structure that models the no-op. It is much cleaner IMO to have to caller explicitly deal with the no-op situation than having a no-op being encoded in data structure values. I dont really see too much of an if-then-else situation on the caller side. Even if it does, it is then explicit in code that the caller is dealing with the no-op situation.

gysit added a comment.Aug 23 2021, 9:38 AM

I added some minor comments that should be orthogonal to where the nop handling happens.


nit: AFAIK there is a constructor taking a single affine expression so the makeArrayRef can be dropped. It may also be an idea to start from an identity map to make this a bit simpler. However, since we need AffineMapAttr there is probably no substantially shorter solution here...


Can we use the indexingMap directly in this case?


nit: As long as this is the single instance I would inline the neutral result generation.


nit: I would probably delete the assertion since we handle the case above?


nit: %0 should probably be replaced by a variable matching the collapse_shape result

tpopp marked 6 inline comments as done.Aug 25 2021, 8:49 AM
tpopp added inline comments.

I'm doing it as you wish, but just to note, this is not making the caller handle the no-op situation. This function handles the no-op case. Then there is a separate unsupported case that results in a no-op. This is making the caller handle the unsupported situation while this function handles the no-op situation.


I did as you suggested for the single affine expression. I did not see any way to start with an identity map, as a given identity map needed to be N->N for any value N in [0,D), and the given functionalities for identity maps created a mapping for every value in a suffix of [0,D) only.

tpopp updated this revision to Diff 368658.Aug 25 2021, 8:49 AM
tpopp marked 2 inline comments as done.

Move the unsupported logic to the caller.

mravishankar accepted this revision.Aug 30 2021, 7:51 PM
This revision is now accepted and ready to land.Aug 30 2021, 7:51 PM
nicolasvasilache accepted this revision.Aug 31 2021, 1:28 AM
This revision was landed with ongoing or failed builds.Aug 31 2021, 3:17 AM
This revision was automatically updated to reflect the committed changes.