Page MenuHomePhabricator

Fix FoldReshapeOpWithUnitExtent when parentSrcType is rank-1
AbandonedPublic

Authored by asaadaldien on Sep 28 2020, 8:58 PM.

Details

Summary

When folding reshapes A -> B -> C and |A| = 1, the association map A -> C is a simple identity.
The current code doesn't handle this case so this diff add it explicitly.

Diff Detail

Event Timeline

asaadaldien created this revision.Sep 28 2020, 8:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 8:58 PM
asaadaldien requested review of this revision.Sep 28 2020, 8:58 PM
mravishankar requested changes to this revision.Sep 29 2020, 10:31 AM
mravishankar added inline comments.
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
407

Does this really need to be here. I think the logic below should cover this case as well. Maybe something is off there.

This revision now requires changes to proceed.Sep 29 2020, 10:31 AM
asaadaldien added inline comments.Sep 29 2020, 11:24 AM
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
407

Why not ? This is a trivial special case for reshapes A -> B -> C with rank(A) = 1 and result always have this form. Its fair to hoist trivial case and catch them up.
FWIW we can add a correctness assertion rank(C) = size(reassociationMaps). then generalize the code below in a case by case.

mravishankar added inline comments.Sep 29 2020, 2:02 PM
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
407

I looked into this and it seems like swapping the if condition at line 447 and line 450 below seems to fix the problem.

I really see this is a bug in the logic below, and unless its something substantial there is no point special casing this.

I have the fix (some changes to the test is needed as well). If you want I can send out the patch or you can update this one.

asaadaldien added inline comments.Sep 29 2020, 2:32 PM
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
407

Thanks! feel free to send the fix you already have it =). I will retire this diff.

However I still think doing this is harmless

if (cond_1) return cheap_cond_1_solution();

// Compute generic solution

assert(generic_solution);
mravishankar added inline comments.Sep 29 2020, 2:52 PM
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
407

Ok, I will send out the patch. If generic_solution handles cond_1 then why bother. If it doesnt then its not "generic" enough. Special casing might end up hiding bugs.

asaadaldien added inline comments.Sep 29 2020, 3:35 PM
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
407

Why bother -> cheaper to compute =).
These kind of transformations without fuzzing infrastructure hard to reason about correctness / coverage.

mravishankar resigned from this revision.Sep 29 2020, 4:20 PM
mravishankar added inline comments.
mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
407

Can be retired in favor of D88521

Why bother -> cheaper to compute =).

These kind of transformations without fuzzing infrastructure hard to reason about correctness / coverage.

Still dont see whats the reason for special casing. If you can generalize the logic (as can be done here) then why not do it. If by cheaper you mean compile time cheaper, this is really negligible here.

This revision now requires review to proceed.Sep 29 2020, 4:20 PM