This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix invalid cast for vector shuffles when lowering to the xxsplti32dx instruction.
ClosedPublic

Authored by amyk on Oct 1 2022, 10:30 PM.

Details

Summary

When lowering vector shuffles into the xxsplti32dx instruction on Power10, we canonicalize the
right operand to be a BUILD_VECTOR and as a result, get the commuted vector shuffle node.

However, a vector shuffle will not always be returned as the result for a commuted vector shuffle.
In such a scenario, this patch updates the original cast of a shuffle into a dyn_cast<> and checks if
the shuffle is a valid vector shuffle node prior to obtaining the commuted shuffle mask.

This patch also adds a new test case that demonstrates this scenario (primarily seen on 32-bit), and
was originally a crash prior to this fix.

Diff Detail

Event Timeline

amyk created this revision.Oct 1 2022, 10:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2022, 10:30 PM
amyk requested review of this revision.Oct 1 2022, 10:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2022, 10:30 PM
ZarkoCA added inline comments.Oct 13 2022, 10:06 AM
llvm/test/CodeGen/PowerPC/p10-splatImm32-undef.ll
35–60

These register spills are not seen with any other target in the test case. Is this maybe an artifact of the test case, or something else? What happens on Linux 64 BE?

nemanjai added inline comments.Oct 13 2022, 11:04 AM
llvm/test/CodeGen/PowerPC/p10-splatImm32-undef.ll
35–60

Seems unrelated to this patch but nonetheless it is bad. @amyk can you please pre-commit the test case?

amyk updated this revision to Diff 467789.Oct 14 2022, 8:54 AM

Rebase the patch to add 32-bit run lines in pre-commit test case (rG22e4203df813a8051b40adb3e2872e30fdbe1bbe)

amyk added inline comments.Oct 14 2022, 8:57 AM
llvm/test/CodeGen/PowerPC/p10-splatImm32-undef.ll
35–60

@ZarkoCA @nemanjai Good catch! Thanks, both of you.

So, I've pre-committed the test case with 64-bit Linux (LE and BE) and AIX lines initially. I could not add the 32-bit run lines since they crash (and this patch aims to fix the 32-bit crash).

As we can see with the 64-bit CHECKs, the spills aren't present anymore and they don't seem to be related to my patch, so maybe there was something causing it before that got fixed?

ZarkoCA accepted this revision.Oct 18 2022, 11:11 AM

Thanks for fixing this and for pre-comitting the test case.

This revision is now accepted and ready to land.Oct 18 2022, 11:11 AM