Page MenuHomePhabricator

[PowerPC] Replace the PPCISD:: SExtVElems with ISD::SIGN_EXTEND_INREG to leverage the combine rules
AcceptedPublic

Authored by steven.zhang on Nov 27 2019, 12:44 AM.

Details

Reviewers
nemanjai
jsji
hfinkel
lkail
Group Reviewers
Restricted Project
Summary

The PPCISD::SExtVElems was added by commit https://reviews.llvm.org/D34009 However, we have another ISD node ISD::SIGN_EXTEND_INREG that perfectly match the semantics of SExtVElems. And the DAGCombiner has some combine rules for SIGN_EXTEND_INREG that produce better code.

Diff Detail

Event Timeline

steven.zhang created this revision.Nov 27 2019, 12:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2019, 12:44 AM

Gentle ping ...

Gentle ping ...

lkail requested changes to this revision.EditedDec 11 2019, 1:46 AM
lkail added a subscriber: lkail.

Can you provide tests to demonstrate how ISD::SIGN_EXTEND_INREG works on PowerPC? Also, from my view, this patch should not be a NFC patch, since we somehow change execution paths, though we finally get the same codegen.

This revision now requires changes to proceed.Dec 11 2019, 1:46 AM
steven.zhang planned changes to this revision.Dec 11 2019, 2:07 AM

Hmm, yes, not NFC as the dag combine rule will applied if with generic isd node. I will cook some tests.

steven.zhang retitled this revision from [NFC][PowerPC] Replace the PPCISD:: SExtVElems with ISD::SIGN_EXTEND_INREG to leverage the combine rules to [PowerPC] Replace the PPCISD:: SExtVElems with ISD::SIGN_EXTEND_INREG to leverage the combine rules .Dec 11 2019, 2:07 AM
jsji added a project: Restricted Project.Thu, Jan 30, 7:18 AM

Rebase the patch. And I think the test case added by https://reviews.llvm.org/D34009 already cover my patch. We are combing the instructions into sext_inreg instead of Power specific node SExtVElems, and then, it is selected as hw instruction which is defined in the pattern td.

Seems that the pre merge checker has problem in detecting the dependent patch landing or not. Remove the parent revision and trigger the check again.

lkail added a comment.EditedMon, Feb 17, 7:32 PM

We are combing the instructions into sext_inreg instead of Power specific node SExtVElems, and then, it is selected as hw instruction which is defined in the pattern td.

CMIIW, SExtVElems works on types that fit in a vector register, like v16i8. And according to codes SExtVElems generates, it intends to sext lower bits of target element type in-place, which suggests SIGN_EXTEND_INREG.

We are combing the instructions into sext_inreg instead of Power specific node SExtVElems, and then, it is selected as hw instruction which is defined in the pattern td.

CMIIW, SExtVElems works on types that fit in a vector register, like v16i8. And according to codes SExtVElems generates, it intends to sext lower bits of target element type in-place, which suggests SIGN_EXTEND_INREG.

Yes, it is doing something like: a << N >> N; The SExtVElems has the same semantics with SIGN_EXTEND_INREG if its type is vector.

lkail accepted this revision.Mon, Feb 17, 9:45 PM

LGTM. Please wait a couple of days to land in case other reviewers have other concerns.

This revision is now accepted and ready to land.Mon, Feb 17, 9:45 PM