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

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 7 15:31:29 PDT 2016


mehdi_amini added inline comments.


================
Comment at: include/llvm/Transforms/Scalar.h:525
+//
+FunctionPass *createCondCallDCEPass(bool OptimizeForSize = false);
 } // End llvm namespace
----------------
xur wrote:
> 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?
That's fine, removing OptimizeForSize addresses it.


================
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
----------------
xur wrote:
> 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.
OK!


================
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(
----------------
xur wrote:
> mehdi_amini wrote:
> > I don't believe it is common to have such flags in pass implementations?
> You mean in PassManagerBuilder?
We have such flags in the pass manager builder, yes. But not here in within the pass itself I think.


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:129
+    if (!Arg->getType()->isFloatTy())
+      V = ConstantExpr::getFPExtend(V, Arg->getType());
+    return BBBuilder.CreateFCmp(Cmp, Arg, V);
----------------
xur wrote:
> 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.
I don't know about what can we stuck into TLI? What would the Cuda target do for instance?


https://reviews.llvm.org/D24414





More information about the llvm-commits mailing list