Page MenuHomePhabricator

[PowerPC] Cust lower fpext v2f32 to v2f64 from extract_subvector v4f32
ClosedPublic

Authored by lei on May 15 2019, 1:36 PM.

Details

Summary

This is a follow up patch from https://reviews.llvm.org/D57857 to handle extract_subvector v4f32.

For cases where we fpext of v2f32 to v2f64 from extract_subvector we currently generate on P9 the following:

lxv 0, 0(3)
xxsldwi 1, 0, 0, 1
xscvspdpn 2, 0
xxsldwi 3, 0, 0, 3
xxswapd 0, 0
xscvspdpn 1, 1
xscvspdpn 3, 3
xscvspdpn 0, 0
xxmrghd 0, 0, 3
xxmrghd 1, 2, 1
stxv 0, 0(4)
stxv 1, 0(5)

This patch custom lower it to the following sequence:

        lxv 0, 0(3)       # load the v4f32 <w0, w1, w2, w3>
	xxmrghw 2, 0, 0   # Produce the following vector <w0, w0, w1, w1>
        xxmrglw 3, 0, 0   # Produce the following vector <w2, w2, w3, w3>
        xvcvspdp 2, 2     # FP-extend to <d0, d1>
	xvcvspdp 3, 3     # FP-extend to <d2, d3>
        stxv 2, 0(5)      # Store <d0, d1> (%vecinit11)
        stxv 3, 0(4)      # Store <d2, d3> (%vecinit4)

Diff Detail

Repository
rL LLVM

Event Timeline

lei created this revision.May 15 2019, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 1:36 PM
lei updated this revision to Diff 199674.May 15 2019, 1:46 PM
nemanjai requested changes to this revision.Jul 5 2019, 11:13 AM

A few minor nits and a functional problem that needs to be addressed (exiting if the input is not a v4f32).

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9632 ↗(On Diff #199674)

Nit: spaces around the ==.

9635 ↗(On Diff #199674)

I think you mean doubleword here.

9637 ↗(On Diff #199674)

Nit: the return SDValue() should be on the line under the if.
We also need a check here that the input to the EXTRACT_SUBVECTOR is actually of type v4f32, otherwise this doesn't work.
If you add a test case much like the one you added here, but change all instances of
<4 x float> to <16 x float>. This will not do the right thing.

9638 ↗(On Diff #199674)

Nit: spaces around :. Also, this is really just Idx >> 1 isn't it?

9641 ↗(On Diff #199674)

Same nit about the code being on the line under the if. And also, not word, but doubleword.

This revision now requires changes to proceed.Jul 5 2019, 11:13 AM
lei updated this revision to Diff 211160.Jul 22 2019, 11:49 AM
lei marked 5 inline comments as done.

Address review comments.

nemanjai requested changes to this revision.Aug 21 2019, 12:02 AM
nemanjai added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9884 ↗(On Diff #211160)

I don't think this assert does what you want it to.
This makes sure that one of the following is true:

  • There are two operands
  • The second operand is a constant
  • The first operand does not have type v4f32

And what you want is that all of those are true (except the last one needs to be flipped):

assert(Op0.getNumOperands() == 2 &&
       isa<ConstantSDNode(Op0.getOperand(1) &&
       Op0.getOperand(0).getValueType == MVT::v4f32 &&
       "Input must be MVT::v4f32 and operand 2 must be a constant!");

But in any case, I think we should not assert that last one - it can be false without anything being broken (i.e. pre-legalize). This should be an early exit here:

if (Op0.getOperand(0).getValueType() != MVT::v4f32)
  return SDValue();
9895 ↗(On Diff #211160)

s/double word/doubleword

llvm/test/CodeGen/PowerPC/reduce_scalarization02.ll
11 ↗(On Diff #211160)

Please add an additional test case where the input type is simply changed from <4 x float> to <16 x float>.

This revision now requires changes to proceed.Aug 21 2019, 12:02 AM
lei updated this revision to Diff 219705.Sep 11 2019, 6:59 AM
lei marked 2 inline comments as done.

Addressed reviewer comments.

lei added a comment.Sep 12 2019, 12:36 PM

@nemanjai All comments have been addressed. Please take another look. Thx!

nemanjai accepted this revision.Sep 13 2019, 5:15 AM

Other than the minor nits that you can feel free to fix on the commit, LGTM.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9632 ↗(On Diff #199674)

Two issues:

  • The nit about spaces around == was not addressed
  • We don't want the || but an && since we want both two operands and a constant second operand. Then the parens are no longer required either.
9935 ↗(On Diff #219705)

Sorry, I should have mentioned this before...
It would be clearer that you're simply flipping the bit if you wrote this as
DWord ^= 0x1 or DWord = DWord ^ 0x1

This revision is now accepted and ready to land.Sep 13 2019, 5:15 AM
lei marked 2 inline comments as done.Sep 13 2019, 10:21 AM
This revision was automatically updated to reflect the committed changes.