[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 ↗

(On Diff #105312)

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 ↗

(On Diff #105312)

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

2244 ↗

(On Diff #105312)

uint16_t Imm

[stefanp]

lib/Target/PowerPC/PPCISelLowering.cpp

2247 ↗

(On Diff #105312)

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 ↗

(On Diff #105312)

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 ↗

(On Diff #105312)

Will update the if condition accordingly

[lei]

lib/Target/PowerPC/PPCISelLowering.cpp

2244 ↗

(On Diff #105312)

The static function isIntS16Immediate() takes a short&

[echristo]

lib/Target/PowerPC/PPCISelLowering.cpp

2244 ↗

(On Diff #105312)

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 ↗

(On Diff #105312)

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 ↗

(On Diff #105312)

fair enough

[lei]

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

[lei]

lib/Target/PowerPC/PPCISelLowering.cpp

2246 ↗

(On Diff #105312)

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

llvm/trunk/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 can reduce
+  // register pressure if we do not materialize a constant just for use as the
+  // index register.  We only get rid of the add if it is not an add of a
+  // value and a 16-bit signed constant and both have a single use.
+  int16_t 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());

llvm/trunk/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

llvm/trunk/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)
 

llvm/trunk/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
+}

llvm/trunk/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