This is an archive of the discontinued LLVM Phabricator instance.

[Matrix] RAUW should only replace an instruction in ShapeMap if supportsShapeInfo
ClosedPublic

Authored by anemet on Jul 23 2021, 2:17 PM.

Details

Summary

As an instruction is replaced in optimizeTransposes RAUW will replace it in
the ShapeMap (ShapeMap is ValueMap so that uses are updated). In
finalizeLowering however we skip updating uses if they are in the ShapeMap
since they will be lowered separately at which point we pick up the lowered
operands.

In the testcase what happened was that since we replaced the doubled-transpose
with the shuffle, it ended up in the ShapeMap. As we lowered the
columnwise-load the use in the shuffle was not updated. Then as we removed
the original columnwise-load we changed that to an undef. I.e. we ended up
with:

%shuf = shufflevector <8 x double> undef, <8 x double> poison, <6 x i32>                                                                                                                                                                 
                                   ^^^^^                                                                                                                                                                                                 
                                  <i32 0, i32 1, i32 2, i32 4, i32 5, i32 6>

Besides the fix itself, I have fortified this last bit. As we change uses to
undef when removing instruction we track the undefed instruction to make sure
we eventually remove those too. This would have caught the issue at compile
time.

Diff Detail

Event Timeline

anemet created this revision.Jul 23 2021, 2:17 PM
anemet requested review of this revision.Jul 23 2021, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2021, 2:17 PM
fhahn added inline comments.Jul 26 2021, 10:40 AM
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
894

Is this to fix a separate issue? The changes seem unrelated to the ShapeInfo issue.

fhahn accepted this revision.Jul 26 2021, 11:35 AM

LGTM thanks! Some suggestions with respect to comments inline. Also, please address the clang tidy issues before committing.

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
894

I see, this is just the extra check mentioned in the commit message! A bit unfortunate that the extra check clutters the code a bit.

I think it might be good to adjust the comment to make it a bit clearer that we effectively replace all uses with undef and only collect/maintain UndefedInst for verification.

908

Might be worth adding a comment here that all undefed instructions also must be removed and if not then this is a hard error.

909

Might be clearer to say '... but instruction not removed'?

This revision is now accepted and ready to land.Jul 26 2021, 11:35 AM
anemet marked 4 inline comments as done.Jul 27 2021, 10:28 AM
This revision was landed with ongoing or failed builds.Jul 27 2021, 11:36 AM
This revision was automatically updated to reflect the committed changes.