[PowerPC] Reduce register pressure by not materializing a constant just for use as an index register for X-Form loads/stores

[lei]

Summary

Reduce register pressure by not materializing a constant just for use as an index register for X-Form loads/stores.

For this example:

float test (int *arr) {
  return arr[2];
}

We currently generate the following code:

li r4, 8
lxsiwax f0, r3, r4
xscvsxdsp f1, f0

With this patch, we will now generate:

addi r3, r3, 8
lxsiwax f0, 0, r3
xscvsxdsp f1, f0

Originally reported in: https://bugs.llvm.org/show_bug.cgi?id=27204

[stefanp]

lib/Target/PowerPC/PPCISelLowering.cpp

2247

Do we need N.getOperand(1) to have only one use?
For example:

r10 = (generate r10 somehow)
ADD r4 = r3 + r10  (Is ADD, is not immed S16, r3 has one use, BUT r10 has two uses)
LD r5 = (0, r4)
ADD r7 = r6 + r10 
LD r8 = (0, r7)

Ideally I think we can get rid of those ADDs from above.
There may be a case I'm not thinking of here.

[echristo]

lib/Target/PowerPC/PPCISelLowering.cpp

2239–2243

Between this and the comment underneath the if conditional it's a little hard to understand the tradeoffs involved. Also Stefan's comment.

2244

uint16_t Imm

[stefanp]

lib/Target/PowerPC/PPCISelLowering.cpp

2247

Sorry... I made a mistake with that last comment.
Ok, so I mixed up the location of the NOT in that if statement.

It may be clearer if instead of ! ( COND && COND && COND) it's !COND || !COND || !COND.
At least it may be clearer in my head...

Either way, I'll have to go back and look at this again.

[lei]

lib/Target/PowerPC/PPCISelLowering.cpp

2239–2243

change:

However we don't want to materialize a constant just for use as the index register.

to:

However, we can reduce register pressure if we do not materialize a constant just for use as the index register.

2247

Will update the if condition accordingly

[lei]

lib/Target/PowerPC/PPCISelLowering.cpp

2244

The static function isIntS16Immediate() takes a short&

[echristo]

lib/Target/PowerPC/PPCISelLowering.cpp

2244

Could fix that too :)

[lei]

Simplify if statement and update comment to be more clear on the tradeoffs.

[jtony]

lib/Target/PowerPC/PPCISelLowering.cpp

2246

It would be nice to break the complex condition into several parts and give an meaningful name for each. It increase the code readability maintainability. Maybe something like this:
bool AAAA = N.getOpcode() == ISD::ADD;
bool BBBB = isIntS16Immediate(N.getOperand(1), imm) && N.getOperand(1).hasOneUse() && N.getOperand(0).hasOneUse(), then we can use
if (AAAA && !BBBB ) {...} instead. As an example name for AAAA, we can use IsADD, and for BBBB: CanGetRidOfAdd. I am not good at naming, you can figure out some better name than I. :-)

[lei]

lib/Target/PowerPC/PPCISelLowering.cpp

2244

fair enough

[lei]

update static function isIntS16Immediate() to take a int16_t param rather then short.

[lei]

lib/Target/PowerPC/PPCISelLowering.cpp

2246

Can't break this up. N.getOperand(1) isn’t guaranteed to succeed, we have to rely on checking whether it’s an ISD::ADD first. Sometimes the address is going to be something like ISD::TargetConstant/ISD::TargetGlobalAddress, etc. which actually doesn’t have operand 1.

[sfertile]

Regarding the isIntS16Immediate change, the function is duplicated in PPCISelDAGToDAG.cpp as well. I suggest we leave it as is in this patch, and then in a separate patch common up the definitions and change it to use an int16_t argument. That keeps this patch focused on only doing a single thing, and puts the code cleanup in its own NFC patch.

[echristo]

In D35027#801294, @sfertile wrote:

Regarding the isIntS16Immediate change, the function is duplicated in PPCISelDAGToDAG.cpp as well. I suggest we leave it as is in this patch, and then in a separate patch common up the definitions and change it to use an int16_t argument. That keeps this patch focused on only doing a single thing, and puts the code cleanup in its own NFC patch.

Feel free to do that first. :)

[hfinkel]

In D35027#801305, @echristo wrote:

In D35027#801294, @sfertile wrote:

Regarding the isIntS16Immediate change, the function is duplicated in PPCISelDAGToDAG.cpp as well. I suggest we leave it as is in this patch, and then in a separate patch common up the definitions and change it to use an int16_t argument. That keeps this patch focused on only doing a single thing, and puts the code cleanup in its own NFC patch.

Feel free to do that first. :)

Yes, please split this into two patches. Change the int16_t first, and then commit the functional change. LGTM.

[lei]

In D35027#801638, @hfinkel wrote:

In D35027#801305, @echristo wrote:

In D35027#801294, @sfertile wrote:

Regarding the isIntS16Immediate change, the function is duplicated in PPCISelDAGToDAG.cpp as well. I suggest we leave it as is in this patch, and then in a separate patch common up the definitions and change it to use an int16_t argument. That keeps this patch focused on only doing a single thing, and puts the code cleanup in its own NFC patch.

Feel free to do that first. :)

Yes, please split this into two patches. Change the int16_t first, and then commit the functional change. LGTM.

NFC patch committed: SVN-307442

lib/Target/PowerPC/PPCISelLowering.cpp

bool PPCTargetLowering::SelectAddressRegRegOnly(SDValue N, SDValue &Base,
                                                 SDValue &Index,
                                                 SelectionDAG &DAG) const {
   // Check to see if we can easily represent this as an [r+r] address.  This
   // will fail if it thinks that the address is more profitably represented as
   // reg+imm, e.g. where imm = 0.
   if (SelectAddressRegReg(N, Base, Index, DAG))
     return true;
 
-  // If the operand is an addition, always emit this as [r+r], since this is
-  // better (for code size, and execution, as the memop does the add for free)
-  // than emitting an explicit add.
-  if (N.getOpcode() == ISD::ADD) {
+  // If the address is the result of an add, we will utilize the fact that the
+  // address calculation includes an implicit add. However we don't want to
+  // materialize a constant just for use as the index register. Unless this is
+  // an add of a value and a 16-bit signed constant and both have a single use,
+  // we get rid of the add.
+  short imm = 0;
+  if (N.getOpcode() == ISD::ADD &&
+      (!(isIntS16Immediate(N.getOperand(1), imm) &&
+         N.getOperand(1).hasOneUse() && N.getOperand(0).hasOneUse()))) {
     Base = N.getOperand(0);
     Index = N.getOperand(1);
     return true;
   }
 
   // Otherwise, do it the hard way, using R0 as the base register.
   Base = DAG.getRegister(Subtarget.isPPC64() ? PPC::ZERO8 : PPC::ZERO,
                          N.getValueType());

test/CodeGen/PowerPC/build-vector-tests.ll

 ; P9BE: vperm
 ; P9BE: blr
 ; P9LE: sldi {{r[0-9]+}}, r4, 2
 ; P9LE-DAG: lxv {{v[0-9]+}}
 ; P9LE-DAG: lxv
 ; P9LE: vperm
 ; P9LE: blr
 ; P8BE: sldi {{r[0-9]+}}, r4, 2
-; P8BE-DAG: lxvw4x {{v[0-9]+}}, r3,
+; P8BE-DAG: lxvw4x {{v[0-9]+}}, 0, r3
 ; P8BE-DAG: lxvw4x
 ; P8BE: vperm
 ; P8BE: blr
 ; P8LE: sldi {{r[0-9]+}}, r4, 2
 ; P8LE-DAG: lxvd2x
 ; P8LE-DAG: lxvd2x
 ; P8LE: xxswapd
 ; P8LE: vperm

 ; P9BE: vperm
 ; P9BE: blr
 ; P9LE-DAG: sldi {{r[0-9]+}}, r4, 2
 ; P9LE-DAG: lxv {{v[0-9]+}}, -12(r3)
 ; P9LE-DAG: lxv
 ; P9LE: vperm
 ; P9LE: blr
 ; P8BE-DAG: sldi {{r[0-9]+}}, r4, 2
-; P8BE-DAG: lxvw4x {{v[0-9]+}}, r3
+; P8BE-DAG: lxvw4x {{v[0-9]+}}, 0, r3
 ; P8BE-DAG: lxvw4x
 ; P8BE: vperm
 ; P8BE: blr
 ; P8LE-DAG: sldi {{r[0-9]+}}, r4, 2
 ; P8LE-DAG: lvx
 ; P8LE-DAG: lvx
 ; P8LE: vperm
 ; P8LE: blr

test/CodeGen/PowerPC/ppc64le-smallarg.ll

 
 define void @caller2() {
 entry:
   %0 = load float, float* @gf, align 4
   %call = tail call float @test2(float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float 0.000000e+00, float %0)
   ret void
 }
 ; CHECK: @caller2
-; CHECK: li [[TOCOFF:[0-9]+]], 136
-; CHECK: stxsspx {{[0-9]+}}, 1, [[TOCOFF]]
+; CHECK: addi [[TOCOFF:[0-9]+]], {{[0-9]+}}, 136
+; CHECK: stxsspx {{[0-9]+}}, 0, [[TOCOFF]]
 ; CHECK: bl test2
 
 declare float @test2(float, float, float, float, float, float, float, float, float, float, float, float, float, float)
 

test/CodeGen/PowerPC/select-addrRegRegOnly.ll

+; RUN: llc -mcpu=pwr8 -mtriple=powerpc64le-unknown-unknown -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mcpu=pwr8 -mtriple=powerpc64-unknown-unknown -verify-machineinstrs < %s | FileCheck %s
+
+; Function Attrs: norecurse nounwind readonly
+define float @testSingleAccess(i32* nocapture readonly %arr) local_unnamed_addr #0 {
+; CHECK-LABEL: testSingleAccess:
+; CHECK:       # BB#0: # %entry
+; CHECK-NEXT:    addi 3, 3, 8
+; CHECK-NEXT:    lxsiwax 0, 0, 3
+; CHECK-NEXT:    xscvsxdsp 1, 0
+; CHECK-NEXT:    blr
+entry:
+  %arrayidx = getelementptr inbounds i32, i32* %arr, i64 2
+  %0 = load i32, i32* %arrayidx, align 4
+  %conv = sitofp i32 %0 to float
+  ret float %conv
+}
+
+; Function Attrs: norecurse nounwind readonly
+define float @testMultipleAccess(i32* nocapture readonly %arr) local_unnamed_addr #0 {
+; CHECK-LABEL: testMultipleAccess:
+; CHECK:       # BB#0: # %entry
+; CHECK-NEXT:    lwz 4, 8(3)
+; CHECK-NEXT:    lwz 12, 12(3)
+; CHECK-NEXT:    add 3, 12, 4
+; CHECK-NEXT:    mtvsrwa 0, 3
+; CHECK-NEXT:    xscvsxdsp 1, 0
+; CHECK-NEXT:    blr
+entry:
+  %arrayidx = getelementptr inbounds i32, i32* %arr, i64 2
+  %0 = load i32, i32* %arrayidx, align 4
+  %arrayidx1 = getelementptr inbounds i32, i32* %arr, i64 3
+  %1 = load i32, i32* %arrayidx1, align 4
+  %add = add nsw i32 %1, %0
+  %conv = sitofp i32 %add to float
+  ret float %conv
+}

test/CodeGen/PowerPC/vsx-partword-int-loads-and-stores.ll

entry:
   %conv = trunc i16 %0 to i8
   %splat.splatinsert = insertelement <16 x i8> undef, i8 %conv, i32 0
   %splat.splat = shufflevector <16 x i8> %splat.splatinsert, <16 x i8> undef, <16 x i32> zeroinitializer
   ret <16 x i8> %splat.splat
 ; CHECK-LABEL: vecucus
 ; CHECK: lxsibzx 34, 0, 3
 ; CHECK-NEXT: vspltb 2, 2, 7
 ; CHECK-BE-LABEL: vecucus
-; CHECK-BE: li [[OFFSET:[0-9]+]], 1
-; CHECK-BE-NEXT: lxsibzx 34, 3, [[OFFSET]]
+; CHECK-BE: addi [[OFFSET:[0-9]+]], [[OFFSET]], 1
+; CHECK-BE-NEXT: lxsibzx 34, 0, [[OFFSET]]
 ; CHECK-BE-NEXT: vspltb 2, 2, 7
 }
 
 ; Function Attrs: norecurse nounwind readonly
 define <8 x i16> @vecusus(i16* nocapture readonly %ptr) {
 entry:
   %0 = load i16, i16* %ptr, align 2
   %splat.splatinsert = insertelement <8 x i16> undef, i16 %0, i32 0
entry:
   %conv = trunc i16 %0 to i8
   %splat.splatinsert = insertelement <16 x i8> undef, i8 %conv, i32 0
   %splat.splat = shufflevector <16 x i8> %splat.splatinsert, <16 x i8> undef, <16 x i32> zeroinitializer
   ret <16 x i8> %splat.splat
 ; CHECK-LABEL: vecscus
 ; CHECK: lxsibzx 34, 0, 3
 ; CHECK-NEXT: vspltb 2, 2, 7
 ; CHECK-BE-LABEL: vecscus
-; CHECK-BE: li [[OFFSET:[0-9]+]], 1
-; CHECK-BE-NEXT: lxsibzx 34, 3, [[OFFSET]]
+; CHECK-BE: addi [[OFFSET:[0-9]+]], [[OFFSET]], 1
+; CHECK-BE-NEXT: lxsibzx 34, 0, [[OFFSET]]
 ; CHECK-BE-NEXT: vspltb 2, 2, 7
 }
 
 ; Function Attrs: norecurse nounwind readonly
 define <8 x i16> @vecssus(i16* nocapture readonly %ptr) {
 entry:
   %0 = load i16, i16* %ptr, align 2
   %splat.splatinsert = insertelement <8 x i16> undef, i16 %0, i32 0
entry:
   %conv = trunc i16 %0 to i8
   %splat.splatinsert = insertelement <16 x i8> undef, i8 %conv, i32 0
   %splat.splat = shufflevector <16 x i8> %splat.splatinsert, <16 x i8> undef, <16 x i32> zeroinitializer
   ret <16 x i8> %splat.splat
 ; CHECK-LABEL: vecucss
 ; CHECK: lxsibzx 34, 0, 3
 ; CHECK-NEXT: vspltb 2, 2, 7
 ; CHECK-BE-LABEL: vecucss
-; CHECK-BE: li [[OFFSET:[0-9]+]], 1
-; CHECK-BE-NEXT: lxsibzx 34, 3, [[OFFSET]]
+; CHECK-BE: addi [[OFFSET:[0-9]+]], [[OFFSET]], 1
+; CHECK-BE-NEXT: lxsibzx 34, 0, [[OFFSET]]
 ; CHECK-BE-NEXT: vspltb 2, 2, 7
 }
 
 ; Function Attrs: norecurse nounwind readonly
 define <4 x i32> @vecuiss(i16* nocapture readonly %ptr) {
 entry:
   %0 = load i16, i16* %ptr, align 2
   %conv = sext i16 %0 to i32
entry:
   %conv = trunc i16 %0 to i8
   %splat.splatinsert = insertelement <16 x i8> undef, i8 %conv, i32 0
   %splat.splat = shufflevector <16 x i8> %splat.splatinsert, <16 x i8> undef, <16 x i32> zeroinitializer
   ret <16 x i8> %splat.splat
 ; CHECK-LABEL: vecscss
 ; CHECK: lxsibzx 34, 0, 3
 ; CHECK-NEXT: vspltb 2, 2, 7
 ; CHECK-BE-LABEL: vecscss
-; CHECK-BE: li [[OFFSET:[0-9]+]], 1
-; CHECK-BE-NEXT: lxsibzx 34, 3, [[OFFSET]]
+; CHECK-BE: addi [[OFFSET:[0-9]+]], [[OFFSET]], 1
+; CHECK-BE-NEXT: lxsibzx 34, 0, [[OFFSET]]
 ; CHECK-BE-NEXT: vspltb 2, 2, 7
 }
 
 ; Function Attrs: norecurse nounwind readonly
 define <4 x i32> @vecsiss(i16* nocapture readonly %ptr) {
 entry:
   %0 = load i16, i16* %ptr, align 2
   %conv = sext i16 %0 to i32