[PATCH] D105236: [PowerPC] Implament Load and Reserve and Store Conditional Builtins

[Stefan Pintilie via Phabricator via cfe-commits]

stefanp added a comment.

Overall I think this is fine. I just have a few nits.



================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat-LoadReseve-StoreCond.c:11
+int test_lwarx(volatile int* a) {
+  // CHECK: entry:
+  // CHECK: %0 = bitcast i32* %a to i8*
----------------
Please check that this "entry:" is printed out when asserts are on and when asserts are off you may want to remove it at this point.

I would prefer you check the name of the function instead of "entry". You can use `CHECK-LABEL` to do that. 


================
Comment at: clang/test/CodeGen/builtins-ppc-xlcompat-error.c:18
+}
+#endif
----------------
This entire test is for 32 bit Power PC. There is only one run line and that is what is specified.
Why are you checking the macros? 


================
Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:1529
+  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">,
----------------
nit:
Does this go past the 80 char limit?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105236