Fix R0 use in PowerPC VSX store for FastIsel

[sfantao]

Summary

Hi all,

I observed that for some constructors, e.g:

struct specific_fpval {

double Val;
specific_fpval(double V) : Val(V) {}

};

the VSX store generated to store V into Val is not specifying any offset register causing %noreg to be generated and R0 to be emitted later on. The semantics of the VSX store (e.g. stdsdx) requires R0 to be used as base if we want zero to be used in the computation of the effective address instead of the content of R0. This patch checks if no index register was generated and forces R0 to be used as base address.

This was causing clang auto-compilation to fail in Debug mode. I understand that VSX support is still work under progress, but I decided to contribute this patch in the event this is something no one else was aware. I included a regression in a separate file.

Thanks!
Samuel

[hfinkel]

There are various places where we call:

MRI.setRegClass(SomeReg, &PPC::GPRC_and_GPRC_NOR0RegClass);

to adjust for this issue. Does calling that on IndexReg help? I suppose I don't understand exactly what's going on here? Is it generally legal to call MIBuilder::addReg with an invalid register operand?

[sfantao]

Before this fix, there were instructions being generated like this:

STXSDX %F1<kill>, %X3<kill>, %noreg, %RM<imp-use>

The code seems be prepared to handle %noreg. R0 was being emitted for %noreg. The problem here is not the register class but the order of the operands. If FastISel were emitting something like:

STXSDX %F1<kill>, %X3<kill>, %ZERO8, %RM<imp-use>

we would still have a problem, as the content of R0 would be taken to compute the effective address of the store. If we have:

STXSDX %F1<kill>, %ZERO8, %X3<kill>, %RM<imp-use>

this works fine as R0 used as based address makes 0 to be used in the computation of the effective address instead of the value contained in R0. We are doing something similar for when UseOffset is set, the immediate has to come before the base register in order to get the right semantics.

Thanks,
Samuel

[hfinkel]

LGTM, thanks!

Can you also look into where the %noreg -> r0 mapping happened? Are we missing an assert somewhere in the backend? As you're well aware, we don't really have a general null register in PPC, and I suspect that anywhere we have a one in an instruction is a bug.

[sfantao]

Committed in r232486!

I will follow up on the %noreg emission in a separate thread.

Thanks!
Samuel

lib/Target/PowerPC/PPCFastISel.cpp

switch (Opc) {
       case PPC::STW : Opc = PPC::STWX;  break;
       case PPC::STB8: Opc = PPC::STBX8; break;
       case PPC::STH8: Opc = PPC::STHX8; break;
       case PPC::STW8: Opc = PPC::STWX8; break;
       case PPC::STD:  Opc = PPC::STDX;  break;
       case PPC::STFS: Opc = PPC::STFSX; break;
       case PPC::STFD: Opc = IsVSFRC ? PPC::STXSDX : PPC::STFDX; break;
     }
-    BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc))
-      .addReg(SrcReg).addReg(Addr.Base.Reg).addReg(IndexReg);
+
+    auto MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc))
+        .addReg(SrcReg);
+
+    // If we have an index register defined we use it in the store inst,
+    // otherwise we use X0 as base as it makes the vector instructions to
+    // use zero in the computation of the effective address regardless the
+    // content of the register.
+    if (IndexReg)
+      MIB.addReg(Addr.Base.Reg).addReg(IndexReg);
+    else
+      MIB.addReg(PPC::ZERO8).addReg(Addr.Base.Reg);
   }
 
   return true;
 }
 
 // Attempt to fast-select a store instruction.
 bool PPCFastISel::SelectStore(const Instruction *I) {
   Value *Op0 = I->getOperand(0);

test/CodeGen/PowerPC/fast-isel-load-store-vsx.ll

+;; There are some known limitations in the VSX support during FastIsel 
+;; (see fast-isel-load-store.ll header). Nevertheless, we are adding some 
+;; regressions here for bugs we fix in the meantime
+; RUN: llc < %s -O0 -fast-isel -mattr=+vsx -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 | FileCheck %s --check-prefix=ELF64VSX
+
+;; The semantics of VSX stores for when R0 is used is different depending on
+;; whether it is used as base or offset. If used as base, the effective
+;; address computation will use zero regardless the content of R0. If used as
+;; offset, the content will be used in the effective address. We observed that
+;; for some constructors, the initialization values were being stored without
+;; any offset register being specified which was causing R0 to be used as offset
+;; in regions where it contained the value in the link register. This regression
+;; verifies that R0 is used as base in these situations.
+
+%SomeStruct = type { double }
+
+; ELF64VSX-LABEL: SomeStructCtor
+define linkonce_odr void @SomeStructCtor(%SomeStruct* %this, double %V) unnamed_addr align 2 {
+entry:
+  %this.addr = alloca %SomeStruct*, align 8
+  %V.addr = alloca double, align 8
+  store %SomeStruct* %this, %SomeStruct** %this.addr, align 8
+; ELF64VSX: stxsdx {{[0-9][0-9]?}}, 0, {{[1-9][0-9]?}}
+  store double %V, double* %V.addr, align 8
+  %this1 = load %SomeStruct*, %SomeStruct** %this.addr
+  %Val = getelementptr inbounds %SomeStruct, %SomeStruct* %this1, i32 0, i32 0
+; ELF64VSX: stxsdx {{[0-9][0-9]?}}, 0, {{[1-9][0-9]?}}
+  %0 = load double, double* %V.addr, align 8
+  store double %0, double* %Val, align 8
+  ret void
+ } 
+
+