[PowerPC] Emit warning when SP is clobbered by asm

[quinnp]

Summary

This patch emits a warning when the stack pointer register (R1) is found in
the clobber list of an inline asm statement. Clobbering the stack pointer is
not supported.

[shchenz]

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp

396

PowerPC overrides getReservedRegs(), why don't we use this function and only warn for R1 even no X1?

[nemanjai]

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp

396

It is unfortunately not as simple as that. We do have some reserved registers that can be clobbered. For example R2 can be clobbered since the compiler will spill and restore it.
There should be a comment to that end in the code. The test should definitely cover X1 in 64-bit mode.

Furthermore, there needs to be a 64-bit test as well.

[quinnp]

Addressing review comments. Added a comment about why we are not using getReservedRegs(). Added implementation for PPC64.

[shchenz]

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp

396

Only marking R1/X1 as reserved for PowerPC should be working. But I still think using getReservedRegs() is a better way. If we reserve some registers which can be allocated for some cases conservatively, maybe we should change the logic in getReservedRegs? For the R2/X2 example, I saw there is a TODO to make it allocatable.

And even we are aggressive to mark R2/X2(all registers in getReservedRegs()) as reserved, we have to choose which one of the below two cases is worse:
1: aggressively mark all registers in getReservedRegs() as reserved. We may give a false warning about the clobber register in the inline assembly. For example, we should be able to clobber R2/X2 in a function that does not access TOC.
2: We only emit warnings for R1/X1 like this patch does. We may miss giving a warning for other reserved registers, for example R13/X13. (I saw a real-world case for this wrong usage of thread pointer in the inline assembly).

Anyhow I will not be a blocker for this patch to get in as it is also right.

[nemanjai]

This needs more work.

1. We need a clang test case similar to clang/test/Misc/inline-asm-clobber-warning.c
2. We need to handle r1 on both 64-bit and 32-bit because x1 is just an internal LLVM name
3. We need to handle sp as an alias for r1 as GCC does

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp

396

While I certainly agree that only focusing on R1 is a bit problematic, false warnings aren't free. There are definitely projects that use inline asm and build with -Werror. Breaking such projects would definitely be an undesirable outcome.

[quinnp]

Adressing review comments. Added register alias sp for the register r1.
Added a front end testcase to test the location information and all of the
aliases for r1. Merged the back end test cases so that we are testing both
r1 and x1 in 32bit and 64bit.

[nemanjai]

LGTM

llvm/lib/Target/PowerPC/PPCRegisterInfo.h

public:
   const MCPhysReg *getCalleeSavedRegs(const MachineFunction *MF) const override;
   const uint32_t *getCallPreservedMask(const MachineFunction &MF,
                                        CallingConv::ID CC) const override;
   const uint32_t *getNoPreservedMask() const override;
 
   void adjustStackMapLiveOutMask(uint32_t *Mask) const override;
 
   BitVector getReservedRegs(const MachineFunction &MF) const override;
+  bool isAsmClobberable(const MachineFunction &MF,
+                        MCRegister PhysReg) const override;
   bool isCallerPreservedPhysReg(MCRegister PhysReg,
                                 const MachineFunction &MF) const override;
 
   // Provide hints to the register allocator for allocating subregisters
   // of primed and unprimed accumulators. For example, if accumulator
   // ACC5 is assigned, we also want to assign UACC5 to the input.
   // Similarly if UACC5 is assigned, we want to assign VSRp10, VSRp11
   // to its inputs.

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp

for (auto Reg : CSR_Altivec_SaveList) {
       }
     }
   }
 
   assert(checkAllSuperRegsMarked(Reserved));
   return Reserved;
 }
 
+bool PPCRegisterInfo::isAsmClobberable(const MachineFunction &MF,
+                                       MCRegister PhysReg) const {
+  // We cannot use getReservedRegs() to find the registers that are not asm
+  // clobberable because there are some reserved registers which can be
+  // clobbered by inline asm. For example, when LR is clobbered, the register is
+  // saved and restored. We will harcode the registers that are not asm
+  // cloberable in this function.
+
+  // The stack pointer (X1/R1) is not clobberable by inline asm
+  return PhysReg != (TM.isPPC64() ? PPC::X1 : PPC::R1);
+}
+
 bool PPCRegisterInfo::requiresFrameIndexScavenging(const MachineFunction &MF) const {
   const PPCSubtarget &Subtarget = MF.getSubtarget<PPCSubtarget>();
   const PPCInstrInfo *InstrInfo =  Subtarget.getInstrInfo();
   const MachineFrameInfo &MFI = MF.getFrameInfo();
   const std::vector<CalleeSavedInfo> &Info = MFI.getCalleeSavedInfo();
 
   LLVM_DEBUG(dbgs() << "requiresFrameIndexScavenging for " << MF.getName()
                     << ".\n");

llvm/test/CodeGen/PowerPC/inline-asm-clobber-warning-64.ll

+; RUN: llc < %s -verify-machineinstrs -mtriple=powerpc64-unknown-unkown \
+; RUN:   -mcpu=pwr7 2>&1 | FileCheck %s
+
+; CHECK: warning: inline asm clobber list contains reserved registers: X1
+; CHECK-NEXT: note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
+define void @test_r1_clobber() {
+entry:
+  call void asm sideeffect "nop", "~{x1}"()
+  ret void
+}

llvm/test/CodeGen/PowerPC/inline-asm-clobber-warning.ll

+; RUN: llc < %s -verify-machineinstrs -mtriple=powerpc-unknown-unkown \
+; RUN:   -mcpu=pwr7 2>&1 | FileCheck %s
+
+; CHECK: warning: inline asm clobber list contains reserved registers: R1
+; CHECK-NEXT: note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
+define void @test_r1_clobber() {
+entry:
+  call void asm sideeffect "nop", "~{r1}"()
+  ret void
+}