[PATCH] D24453: Simplify Libcalls for ARM PCS

[James Molloy via llvm-commits]

jmolloy added a subscriber: jmolloy.
jmolloy requested changes to this revision.
jmolloy added a reviewer: jmolloy.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Sam,

Thanks for doing this. The testing in particular is really nice. I have a couple of superficial comments.

Cheers,

James


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:59
@@ -58,1 +58,3 @@
 
+static bool isCallingConvCompatiable(CallInst *CI) {
+  llvm::CallingConv::ID CC = CI->getCallingConv();
----------------
Typo: "compatible"

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:59
@@ -58,1 +58,3 @@
 
+static bool isCallingConvCompatiable(CallInst *CI) {
+  llvm::CallingConv::ID CC = CI->getCallingConv();
----------------
jmolloy wrote:
> Typo: "compatible"
It'd be nice for the function to encode that it's talking about compatibility with the C calling convention. I understand that that would put a lot of C's in a row though...

    isCallingConvCCompatible()

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:64
@@ +63,3 @@
+
+  if (CC == llvm::CallingConv::ARM_APCS ||
+      CC == llvm::CallingConv::ARM_AAPCS ||
----------------
I think this would look neater as a switch.

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:68
@@ +67,3 @@
+
+    if (Triple(CI->getModule()->getTargetTriple()).isiOS())
+      return false;
----------------
Please add a comment as to why.

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:78
@@ +77,3 @@
+
+    for (auto param : FuncTy->params()) {
+      if (param->isPointerTy() || param->isIntegerTy())
----------------
param -> Param

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:79
@@ +78,3 @@
+    for (auto param : FuncTy->params()) {
+      if (param->isPointerTy() || param->isIntegerTy())
+        continue;
----------------
This could simply be:

    if (!P->isPointerTy() && !P->isIntegerTy())
      return false;


https://reviews.llvm.org/D24453