[PATCH] D24414: Conditionally eliminate library calls where the result value is not used

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

That's a smart transformation!
The patch looks good overall, I only have minor comments inline.



================
Comment at: include/llvm/Transforms/Scalar.h:525
+//
+FunctionPass *createCondCallDCEPass(bool OptimizeForSize = false);
 } // End llvm namespace
----------------
I'm not super convinced by the "DCE" in the name, there does not seem to be "dead code" here, and no "code elimination" either.

Also you need to document the parameter `OptimizeForSize`, which I'm not convinced should be there at all. If I read correctly the code it totally disable this pass. So adding a pass to the pipeline while disabling it at the same time does not make sense to me.
Also, it handled the function attribute, isn't it enough?


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:15
+//    if (val < 0)
+//      sqrt(val);
+//  Even if the result of library call is not being used, the compiler cannot
----------------
Can you turn this into "if (val < 0) { errno = ... }" and eliminate the call?
Maybe we don't have guarantee that errno is available?


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:52
+    "cond-call-dce-disable", cl::init(false), cl::Hidden,
+    cl::desc("Disable conditional call Dead Code Elimination"));
+static cl::opt<bool> CondCallDCEDoDomainError(
----------------
I don't believe it is common to have such flags in pass implementations?


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:129
+    if (!Arg->getType()->isFloatTy())
+      V = ConstantExpr::getFPExtend(V, Arg->getType());
+    return BBBuilder.CreateFCmp(Cmp, Arg, V);
----------------
What about FP16 here?


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:314
+  // errno). We can use the same framework to direct calls that known
+  // errno-free to the new API calls.
+  if (!CI.use_empty())
----------------
That's a neat idea!


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:325
+  if (CI.getCallingConv() != llvm::CallingConv::C)
+    return;
+  WorkList.push_back(&CI);
----------------
Any example in mind?

Also there is no test for any of the bailout here. Can you add some?


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:518
+  TLI.getLibFunc(*Callee, Func);
+  assert(Func);
+
----------------
Usually we try to add a message in assertion, something like "`checkCandidate` shouldn't queue non-TLI func"


https://reviews.llvm.org/D24414