[PATCH] D105194: [PowerPC] Add PowerPC cmpb builtin and emit target indepedent code for XL compatibility

[Nemanja Ivanovic via Phabricator via llvm-commits]

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

Mostly minor comments, but it'll be good to have another look to make sure they're all addressed.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15101
+    // ret i64 %or
+    else {
+      Function *F =
----------------
Nit: no `else` after `return`.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15104
+          CGM.getIntrinsic(Intrinsic::ppc_cmpb, {Int32Ty, Int32Ty, Int32Ty});
+      // %conv  = trunc i64 %a to i32
+      Value *A = Builder.CreateTrunc(Ops[0], Int32Ty);
----------------
These comments are superfluous. You have the entire sequence listed above, no need to repeat it piece-wise here.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15105
+      // %conv  = trunc i64 %a to i32
+      Value *A = Builder.CreateTrunc(Ops[0], Int32Ty);
+      // %conv1 = trunc i64 %b to i32
----------------
Please do not name variables with non-descriptive single letters. It is somewhat conventional to use suffixes such as `Hi/Lo` for high/low order parts of a value.


================
Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:1537
+  def int_ppc_cmpb
+      : Intrinsic<[llvm_anyint_ty], [llvm_anyint_ty, llvm_anyint_ty], [IntrNoMem]>;
   // multiply
----------------
Line too long?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105194