This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix vperm codegen
ClosedPublic

Authored by maryammo on Nov 25 2022, 2:13 PM.

Details

Summary

Commit rG934d5fa2b8672695c335deed0e19d0e777c98403 changed the vperm codegen
for cases that vperm is not replaced by xxperm, this patch is to revert that.

Diff Detail

Event Timeline

maryammo created this revision.Nov 25 2022, 2:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2022, 2:13 PM
maryammo requested review of this revision.Nov 25 2022, 2:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2022, 2:13 PM

Thank you for fixing this.
I do have a couple of comments related to the fix.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10204

I don't think you need to bother with this extra ternary operator.
Everywhere you use V1HasXXSWAPD or V2HasXXSWAPD seems to already be guarded by Opcode == PPCISD::XXPERM so it doesn't matter what value those two bool values have in cases when you are not using the XXPERM oeprand.

10265

For this section how about:

if (Opcode == PPCISD::XXPERM && (V1HasXXSWAPD || V2HasXXSWAPD)) {
  /// Same code as above but you don't need to check that (V1HasXXSWAPD || V2HasXXSWAPD)
}

You don't need to go into the if statement at all if there isn't at least one XXSWAPD.

10269

Is it possible to have a swap on both sides?

maryammo added inline comments.Nov 29 2022, 10:18 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10269

I have not see such a case.
btw, this patch is to address the vperm issue, so we can look at the swap on both sides for xxperm if needed separately. Thanks.

maryammo updated this revision to Diff 478632.Nov 29 2022, 10:29 AM

Address the review comments

stefanp accepted this revision.Nov 29 2022, 11:49 AM

Thank you for fixing this!
LGTM.

This revision is now accepted and ready to land.Nov 29 2022, 11:49 AM
maryammo updated this revision to Diff 478709.Nov 29 2022, 1:40 PM

Address the review comments

This revision was landed with ongoing or failed builds.Nov 29 2022, 1:47 PM
This revision was automatically updated to reflect the committed changes.

@maryammo Please can you take a look at https://github.com/llvm/llvm-project/issues/61315 - it looks related to this patch (and D133700)