[PowerPC] Fix L[D|W]ARX Implementation

[Conanap]

Summary

LDARX and LWARX sometimes gets optimized out by the compiler
when it is critical to the correctness of the code. This inline asm generation
ensures that it preserved.

[Conanap]

Update test cases

[Conanap]

Added modifier to $1

[Conanap]

Fixed more test cases

[lkail]

llvm/include/llvm/IR/IntrinsicsPowerPC.td

1535

Just curious, compiler optimizes the instruction out even setting IntrArgMemOnly and IntrReadMem here?

[nemanjai]

llvm/include/llvm/IR/IntrinsicsPowerPC.td

1535

Yes. Well, those two more or less tell it to go ahead and optimize it out if there are no uses. What is more surprising is that even with IntrHasSideEffects. Heck, it even optimizes with all of:

IntrReadMem
IntrArgMemOnly
IntrHasSideEffects
Throws
IntrNoDuplicate
IntrNoMerge

I certainly found that surprising but I assume there is some justification as to why that is. There is some precedent for producing inline asm for builtins (see EmitX86BitTestIntrinsic() and similar). I don't know why that intrinsic was implemented using inline asm (commit messages don't say) but I presume it might have been for a similar reason.

[Conanap]

looking at the failing test cases, for example ./clang/test/CodeGen/RISCV/rvv-intrinsics-overloaded/vadd.c, which are not test cases compiled for PPC, I'm seeing this following error:

clang: /home/conanap/llvm/ccom/llvm-project/llvm/lib/IR/Instructions.cpp:494: void llvm::CallInst::init(llvm::FunctionType *, llvm::Value *, ArrayRef<llvm::Value *>, ArrayRef<llvm::OperandBundleDef>, const llvm::Twine &): Assertion `(i >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType()) && "Calling a function with a bad signature!"' failed.

Debug prints show that it's entering the code block emitLoadReserveIntrinsic and BuiltinID evaluates to clang::PPC::BI__builtin_ppc_lwarx. The FTy dump shows i32 (i32 *), which is correct... I'm not exactly sure what to make of this yet, just putting this update here.

[lkail]

clang/lib/CodeGen/CGBuiltin.cpp

997

Maybe rename to emitPPCLoadReserveIntrinsic should be more appropriate, since other targets also have similar load-reserve/load-linked/load-acquire notions and what this function does is ad hoc to PPC.

[lkail]

clang/lib/CodeGen/CGBuiltin.cpp

1015

Adding default: llvm_unreachable would be nice.

[nemanjai]

I believe that your failing test case is because you are attempting to emit code for these builtins in the target independent code and the values just happen to match up.

clang/lib/CodeGen/CGBuiltin.cpp

997

+1

1000

Nit: please follow naming conventions. Variables are to be capitalized.

1015

+1

5146

Why?
Everything else is Builtin::BI__<builtin_name> but these are
clang::PPC::BI__<builtin_name> for some reason.

That doesn't really make sense to me.
Also, what on earth is this doing here? This is a PPC builtin. Those should be handled in EmitPPCBuiltinExpr() and not here.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll

18–19

This is not the asm that the front end generates. Why would you generate one thing in the front end and then test a different thing in the back end?

[Conanap]

Moved implementation to a more appropriate function,
updated test cases.

[Conanap]

Removed unintended change

[Conanap]

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll

18–19

I'm not quite sure what you mean by this; the IR output is taken from the .c test case above.

[nemanjai]

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll

18–19

I don't think that is the case.
Above:
call i64 asm sideeffect "ldarx $0, ${1:y}", "=r,*Z,~{memory}"(i64* %a)
Here:
call i64 asm sideeffect "ldarx $0, $1", "=r,*Z,~{memory}"(i64* %a)

[Conanap]

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll

18–19

ah I see I missed the modifier.

[Conanap]

Updated a test case

[Conanap]

Updated lwarx test case with modifier

[nemanjai]

LGTM.

clang/lib/CodeGen/CGBuiltin.cpp

static llvm::Value *EmitBitTestIntrinsic(CodeGenFunction &CGF,
 
   // However we loaded the old byte, either by plain load or atomicrmw, shift
   // the bit into the low position and mask it to 0 or 1.
   Value *ShiftedByte = CGF.Builder.CreateLShr(OldByte, PosLow, "bittest.shr");
   return CGF.Builder.CreateAnd(
       ShiftedByte, llvm::ConstantInt::get(CGF.Int8Ty, 1), "bittest.res");
 }
 
+static llvm::Value *emitPPCLoadReserveIntrinsic(CodeGenFunction &CGF,
+                                                unsigned BuiltinID,
+                                                const CallExpr *E) {
+  Value *Addr = CGF.EmitScalarExpr(E->getArg(0));
+
+  SmallString<64> Asm;
+  raw_svector_ostream AsmOS(Asm);
+  llvm::IntegerType *RetType = CGF.Int32Ty;
+
+  switch (BuiltinID) {
+  case clang::PPC::BI__builtin_ppc_ldarx:
+    AsmOS << "ldarx ";
+    RetType = CGF.Int64Ty;
+    break;
+  case clang::PPC::BI__builtin_ppc_lwarx:
+    AsmOS << "lwarx ";
+    RetType = CGF.Int32Ty;
+    break;
+  default:
+    llvm_unreachable("Expected only PowerPC load reserve intrinsics");
+  }
+
+  AsmOS << "$0, ${1:y}";
+
+  std::string Constraints = "=r,*Z,~{memory}";
+  std::string MachineClobbers = CGF.getTarget().getClobbers();
+  if (!MachineClobbers.empty()) {
+    Constraints += ',';
+    Constraints += MachineClobbers;
+  }
+
+  llvm::Type *IntPtrType = RetType->getPointerTo();
+  llvm::FunctionType *FTy =
+      llvm::FunctionType::get(RetType, {IntPtrType}, false);
+
+  llvm::InlineAsm *IA =
+      llvm::InlineAsm::get(FTy, Asm, Constraints, /*hasSideEffects=*/true);
+  return CGF.Builder.CreateCall(IA, {Addr});
+}
+
 namespace {
 enum class MSVCSetJmpKind {
   _setjmpex,
   _setjmp3,
   _setjmp
 };
 }
 
auto Name = CGM.getCUDARuntime().getDeviceSideName(
         cast<DeclRefExpr>(E->getArg(0)->IgnoreImpCasts())->getDecl());
     auto Str = CGM.GetAddrOfConstantCString(Name, "");
     llvm::Constant *Zeros[] = {llvm::ConstantInt::get(SizeTy, 0),
                                llvm::ConstantInt::get(SizeTy, 0)};
     auto *Ptr = llvm::ConstantExpr::getGetElementPtr(Str.getElementType(),
                                                      Str.getPointer(), Zeros);
     return RValue::get(Ptr);
   }
   }
 
   // If this is an alias for a lib function (e.g. __builtin_sin), emit
   // the call using the normal call path, but using the unmangled
   // version of the function name.
   if (getContext().BuiltinInfo.isLibFunction(BuiltinID))
     return emitLibraryCall(*this, FD, E,
                            CGM.getBuiltinLibFunction(FD, BuiltinID));
 
case PPC::BI__builtin_ppc_fetch_and_orlp: {
     return MakeBinaryAtomicValue(*this, AtomicRMWInst::Or, E,
                                  llvm::AtomicOrdering::Monotonic);
   }
   case PPC::BI__builtin_ppc_fetch_and_swap:
   case PPC::BI__builtin_ppc_fetch_and_swaplp: {
     return MakeBinaryAtomicValue(*this, AtomicRMWInst::Xchg, E,
                                  llvm::AtomicOrdering::Monotonic);
   }
+  case PPC::BI__builtin_ppc_ldarx:
+  case PPC::BI__builtin_ppc_lwarx:
+    return emitPPCLoadReserveIntrinsic(*this, BuiltinID, E);
   }
 }
 
 namespace {
 // If \p E is not null pointer, insert address space cast to match return
 // type of \p E if necessary.
 Value *EmitAMDGPUDispatchPtr(CodeGenFunction &CGF,
                              const CallExpr *E = nullptr) {

clang/test/CodeGen/builtins-ppc-xlcompat-LoadReseve-StoreCond-64bit-only.c

 // RUN: not %clang_cc1 -triple=powerpc-unknown-aix -emit-llvm %s -o - 2>&1 |\
 // RUN: FileCheck %s --check-prefix=CHECK32-ERROR
-// RUN: %clang_cc1 -triple=powerpc64-unknown-aix -emit-llvm %s -o - | \
+// RUN: %clang_cc1 -O2 -triple=powerpc64-unknown-aix -emit-llvm %s -o - | \
 // RUN: FileCheck %s --check-prefix=CHECK64
-// RUN: %clang_cc1 -triple=powerpc64le-unknown-unknown -emit-llvm %s \
+// RUN: %clang_cc1 -O2 -triple=powerpc64le-unknown-unknown -emit-llvm %s \
 // RUN:  -o - | FileCheck %s --check-prefix=CHECK64
-// RUN: %clang_cc1 -triple=powerpc64-unknown-unknown -emit-llvm %s \
+// RUN: %clang_cc1 -O2 -triple=powerpc64-unknown-unknown -emit-llvm %s \
 // RUN:  -o - | FileCheck %s --check-prefix=CHECK64
 
 long test_ldarx(volatile long* a) {
   // CHECK64-LABEL: @test_ldarx
-  // CHECK64: %0 = load i64*, i64** %a.addr, align 8
-  // CHECK64: %1 = bitcast i64* %0 to i8*
-  // CHECK64: %2 = call i64 @llvm.ppc.ldarx(i8* %1)
+  // CHECK64: %0 = tail call i64 asm sideeffect "ldarx $0, ${1:y}", "=r,*Z,~{memory}"(i64* %a)
   // CHECK32-ERROR: error: this builtin is only available on 64-bit targets
   return __ldarx(a);
 }
 
 int test_stdcx(volatile long* addr, long val) {
   // CHECK64-LABEL: @test_stdcx
-  // CHECK64: %0 = load i64*, i64** %addr.addr, align 8
-  // CHECK64: %1 = bitcast i64* %0 to i8*
-  // CHECK64: %2 = load i64, i64* %val.addr, align 8
-  // CHECK64: %3 = call i32 @llvm.ppc.stdcx(i8* %1, i64 %2)
+  // CHECK64: %0 = bitcast i64* %addr to i8*
+  // CHECK64: %1 = tail call i32 @llvm.ppc.stdcx(i8* %0, i64 %val)
   // CHECK32-ERROR: error: this builtin is only available on 64-bit targets
   return __stdcx(addr, val);
 }

clang/test/CodeGen/builtins-ppc-xlcompat-LoadReseve-StoreCond.c

-// RUN: %clang_cc1 -triple=powerpc-unknown-aix -emit-llvm %s -o - | \
+// RUN: %clang_cc1 -O2 -triple=powerpc-unknown-aix -emit-llvm %s -o - | \
 // RUN: FileCheck %s
-// RUN: %clang_cc1 -triple=powerpc64-unknown-aix -emit-llvm %s -o - | \
+// RUN: %clang_cc1 -O2 -triple=powerpc64-unknown-aix -emit-llvm %s -o - | \
 // RUN: FileCheck %s
-// RUN: %clang_cc1 -triple=powerpc64le-unknown-unknown -emit-llvm %s \
+// RUN: %clang_cc1 -O2 -triple=powerpc64le-unknown-unknown -emit-llvm %s \
 // RUN:  -o - | FileCheck %s
-// RUN: %clang_cc1 -triple=powerpc64-unknown-unknown -emit-llvm %s \
+// RUN: %clang_cc1 -O2 -triple=powerpc64-unknown-unknown -emit-llvm %s \
 // RUN:  -o - | FileCheck %s
 
 int test_lwarx(volatile int* a) {
   // CHECK: @test_lwarx
-  // CHECK: %1 = bitcast i32* %0 to i8*
-  // CHECK: %2 = call i32 @llvm.ppc.lwarx(i8* %1)
+  // CHECK: %0 = tail call i32 asm sideeffect "lwarx $0, ${1:y}", "=r,*Z,~{memory}"(i32* %a)
   return __lwarx(a);
 }
 int test_stwcx(volatile int* a, int val) {
   // CHECK: @test_stwcx
-  // CHECK: %1 = bitcast i32* %0 to i8*
-  // CHECK: %2 = load i32, i32* %val.addr, align 4
-  // CHECK: %3 = call i32 @llvm.ppc.stwcx(i8* %1, i32 %2)
+  // CHECK: %0 = bitcast i32* %a to i8*
+  // CHECK: %1 = tail call i32 @llvm.ppc.stwcx(i8* %0, i32 %val)
   return __stwcx(a, val);
 }

llvm/include/llvm/IR/IntrinsicsPowerPC.td

let TargetPrefix = "ppc" in {
   def int_ppc_iospace_eieio : GCCBuiltin<"__builtin_ppc_iospace_eieio">,
                               Intrinsic<[],[],[]>;
   def int_ppc_stdcx : GCCBuiltin<"__builtin_ppc_stdcx">,
                       Intrinsic<[llvm_i32_ty], [llvm_ptr_ty, llvm_i64_ty],
                                 [IntrWriteMem]>;
   def int_ppc_stwcx : GCCBuiltin<"__builtin_ppc_stwcx">,
                       Intrinsic<[llvm_i32_ty], [llvm_ptr_ty, llvm_i32_ty],
                                 [IntrWriteMem]>;
-  def int_ppc_lwarx : GCCBuiltin<"__builtin_ppc_lwarx">,
-                      Intrinsic<[llvm_i32_ty], [llvm_ptr_ty], [IntrNoMem]>;
-  def int_ppc_ldarx : GCCBuiltin<"__builtin_ppc_ldarx">,
-                      Intrinsic<[llvm_i64_ty], [llvm_ptr_ty], [IntrNoMem]>;
 }
 

llvm/lib/Target/PowerPC/PPCInstr64Bit.td

def SLBIEG : XForm_26<31, 466, (outs), (ins gprc:$RS, gprc:$RB),
                       "slbieg $RS, $RB", IIC_SprSLBIEG, []>;
 // SLB Synchronize
 def SLBSYNC : XForm_0<31, 338, (outs), (ins), "slbsync", IIC_SprSLBSYNC, []>;
 
 } // IsISA3_0
 
 def : Pat<(int_ppc_stdcx ForceXForm:$dst, g8rc:$A),
           (STDCX g8rc:$A, ForceXForm:$dst)>;
-def : Pat<(int_ppc_ldarx ForceXForm:$dst),
-          (LDARX ForceXForm:$dst)>;

llvm/lib/Target/PowerPC/PPCInstrInfo.td

dag DWord =
     (i64 (INSERT_SUBREG (i64 (IMPLICIT_DEF)), Word, sub_32));
 }
 
 // Now both high word and low word are reversed, next
 // swap the high word and low word.
 def : Pat<(i64 (bitreverse i64:$A)),
   (OR8 (RLDICR DWBytes7654.DWord, 32, 31), DWBytes3210.DWord)>;
 
-def : Pat<(int_ppc_lwarx ForceXForm:$dst),
-          (LWARX ForceXForm:$dst)>;
 def : Pat<(int_ppc_stwcx ForceXForm:$dst, gprc:$A),
           (STWCX gprc:$A, ForceXForm:$dst)>;

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
 ; RUN:   -mcpu=pwr8 < %s | FileCheck %s --check-prefix=CHECK
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu \
 ; RUN:   -mcpu=pwr8 < %s | FileCheck %s --check-prefix=CHECK
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix \
 ; RUN:   -mcpu=pwr8 < %s | FileCheck %s --check-prefix=CHECK
 
 declare i64 @llvm.ppc.ldarx(i8*)
 define dso_local i64 @test_ldarx(i64* readnone %a) {
 ; CHECK-LABEL: test_ldarx:
 ; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    ldarx 3, 0, 3
+; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:    blr
 entry:
-  %0 = bitcast i64* %a to i8*
-  %1 = tail call i64 @llvm.ppc.ldarx(i8* %0)
-  ret i64 %1
+  %0 = call i64 asm sideeffect "ldarx $0, ${1:y}", "=r,*Z,~{memory}"(i64* %a)
+  ret i64 %0
 }
 
 declare i32 @llvm.ppc.stdcx(i8*, i64)
-define dso_local i64 @test(i64* %a, i64 %b) {
-; CHECK-LABEL: test:
+define dso_local i64 @test_stdcx(i64* %a, i64 %b) {
+; CHECK-LABEL: test_stdcx:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    stdcx. 4, 0, 3
 ; CHECK-NEXT:    mfocrf 3, 128
 ; CHECK-NEXT:    srwi 3, 3, 28
 ; CHECK-NEXT:    extsw 3, 3
 ; CHECK-NEXT:    blr
 entry:
   %0 = bitcast i64* %a to i8*
   %1 = tail call i32 @llvm.ppc.stdcx(i8* %0, i64 %b)
   %conv = sext i32 %1 to i64
   ret i64 %conv
 }

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
 ; RUN:   -mcpu=pwr8 < %s | FileCheck %s --check-prefix=CHECK-64
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu \
 ; RUN:   -mcpu=pwr8 < %s | FileCheck %s --check-prefix=CHECK-64
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-aix \
 ; RUN:   -mcpu=pwr8 < %s | FileCheck %s --check-prefix=CHECK-32
 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-aix \
 ; RUN:   -mcpu=pwr8 < %s | FileCheck %s --check-prefix=CHECK-64
 
 declare i32 @llvm.ppc.lwarx(i8*)
 define dso_local signext i32 @test_lwarx(i32* readnone %a) {
 ; CHECK-64-LABEL: test_lwarx:
 ; CHECK-64:       # %bb.0: # %entry
+; CHECK-64-NEXT:    #APP
 ; CHECK-64-NEXT:    lwarx 3, 0, 3
+; CHECK-64-NEXT:    #NO_APP
 ; CHECK-64-NEXT:    extsw 3, 3
 ; CHECK-64-NEXT:    blr
 ;
 ; CHECK-32-LABEL: test_lwarx:
 ; CHECK-32:       # %bb.0: # %entry
+; CHECK-32-NEXT:    #APP
 ; CHECK-32-NEXT:    lwarx 3, 0, 3
+; CHECK-32-NEXT:    #NO_APP
 ; CHECK-32-NEXT:    blr
 entry:
-  %0 = bitcast i32* %a to i8*
-  %1 = tail call i32 @llvm.ppc.lwarx(i8* %0)
-  ret i32 %1
+  %0 = call i32 asm sideeffect "lwarx $0, $1", "=r,*Z,~{memory}"(i32* %a)
+  ret i32 %0
 }
 
 declare i32 @llvm.ppc.stwcx(i8*, i32)
 define dso_local signext i32 @test_stwcx(i32* %a, i32 signext %b) {
 ; CHECK-64-LABEL: test_stwcx:
 ; CHECK-64:       # %bb.0: # %entry
 ; CHECK-64-NEXT:    stwcx. 4, 0, 3
 ; CHECK-64-NEXT:    mfocrf 3, 128