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

[Rong Xu via llvm-commits]

xur marked 3 inline comments as done.
xur added a comment.

Thanks for reviewing this patch. I'll update with your comments and upload it soon.



================
Comment at: include/llvm/Transforms/Scalar.h:525
+//
+FunctionPass *createCondCallDCEPass(bool OptimizeForSize = false);
 } // End llvm namespace
----------------
mehdi_amini wrote:
> 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?
The intention is to eliminate the call in the runtime. But I agree that there should be a better name for this. Let me think about this.

Thanks for finding OptimizeForSize issue. In the earlier implementation,  there is a part of pass still doing some work with OptimizeForSize==true . Now that part is removed from the pass which makes the OptimizeForSize useless. I'll remove this part.

OptimizeForSize is passed from pass manager and it also reads the function attribute. I'm not sure I understand your last question. Can you explain more?


================
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
----------------
mehdi_amini wrote:
> Can you turn this into "if (val < 0) { errno = ... }" and eliminate the call?
> Maybe we don't have guarantee that errno is available?
I can do this for some calls -- if I can use the exact condition for the call. For others, like pow(), the conditions we used here are the superset of the true conditions of generating errno . We can not set the errno directly.

Also the assumption is that the chance of getting to the err code path is low. We probably don't concern the performance of this cold path.  I would prefer to keep as it is.


================
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(
----------------
mehdi_amini wrote:
> I don't believe it is common to have such flags in pass implementations?
You mean in PassManagerBuilder?


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:129
+    if (!Arg->getType()->isFloatTy())
+      V = ConstantExpr::getFPExtend(V, Arg->getType());
+    return BBBuilder.CreateFCmp(Cmp, Arg, V);
----------------
mehdi_amini wrote:
> What about FP16 here?
Are there math lib calls using FP16 type?
I only know the following types for the math lib calls: double, float and long double.


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:325
+  if (CI.getCallingConv() != llvm::CallingConv::C)
+    return;
+  WorkList.push_back(&CI);
----------------
mehdi_amini wrote:
> Any example in mind?
> 
> Also there is no test for any of the bailout here. Can you add some?
We probably do not need this check.
Let me try to remove this.


https://reviews.llvm.org/D24414