[PATCH] D106130: [PowerPC] Implemented mtmsr, mfspr, mtspr Builtins

Fri Jul 16 05:24:18 PDT 2021

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

Why does this review have no reviewers listed?

================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:15586
+  case PPC::BI__builtin_ppc_mfspr: {
+    llvm::Type *RetType = CGM.getDataLayout().getTypeSizeInBits(VoidPtrTy) == 32
+                              ? Int32Ty
----------------
Is this the formatting that `clang-format` produces? Seems surprising it would format it that way.

================
Comment at: clang/lib/Sema/SemaChecking.cpp:3374
+  case PPC::BI__builtin_ppc_mfspr:
+    return SemaBuiltinConstantArgRange(TheCall, 0, 1, 898);
 #define CUSTOM_BUILTIN(Name, Intr, Types, Acc) \
----------------
I don't think we should enforce the range. The architecture may add more SPR's in the future and then this check will need to be updated. Just ensure that the register number (as well as the value for `mtspr`) are constants.

================
Comment at: llvm/include/llvm/IR/IntrinsicsPowerPC.td:1581
                       Intrinsic<[llvm_i32_ty], [], [IntrNoMem]>;
+  def int_ppc_mfspr : Intrinsic<[llvm_anyint_ty], [llvm_i32_ty], [ImmArg<ArgIndex<0>>]>;
+  def int_ppc_mtmsr
----------------
Nit: line too long

================
Comment at: llvm/lib/Target/PowerPC/PPCInstr64Bit.td:415

-
 //===----------------------------------------------------------------------===//
----------------
Unrelated whitespace change.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:5442
 def : Pat<(int_ppc_mftbu), (MFTB 269)>;
+def : Pat<(i32 (int_ppc_mfspr i32:$SPR)),
+          (MFSPR $SPR)>;
----------------
Shouldn't this be `imm` instead of `i32`?
Have you tried compiling the test cases with `-filetype=obj` and/or with `-ppc-asm-full-reg-names`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106130