[PATCH] D105754: [PowerPC] Fix L[D|W]ARX Implementation

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:997
 
+static llvm::Value *emitLoadReserveIntrinsic(CodeGenFunction &CGF,
+                                             unsigned BuiltinID,
----------------
lkail wrote:
> 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.
+1


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1000
+                                             const CallExpr *E) {
+  Value *addr = CGF.EmitScalarExpr(E->getArg(0));
+
----------------
Nit: please follow naming conventions. Variables are to be capitalized.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1015
+    break;
+  }
+
----------------
lkail wrote:
> Adding `default: llvm_unreachable` would be nice.
+1


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5144
   }
+  case clang::PPC::BI__builtin_ppc_ldarx:
+  case clang::PPC::BI__builtin_ppc_lwarx:
----------------
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.


================
Comment at: llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond-64bit-only.ll:18
 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", "=r,*Z,~{memory}"(i64* %a)
+  ret i64 %0
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105754/new/

https://reviews.llvm.org/D105754