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

David Li via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 16 14:20:01 PDT 2016


davidxl added inline comments.

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:10
@@ +9,3 @@
+//
+// This pass shrink-wraps a condition to some library calls where the call
+// result is not used. For example:
----------------
shrink wraps a call to function .. if the result is not used. The call can set errno but is otherwise side effect free.

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:54
@@ +53,3 @@
+    cl::desc("Disable conditional call Dead Code Elimination"));
+static cl::opt<bool> CondCallDCEDoNoError(
+    "cond-call-dce-no-error", cl::init(true), cl::Hidden,
----------------
The name of this option is quite confusing. Perhaps DoDeleteCallNoErrNo?

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:98
@@ +97,3 @@
+private:
+  struct CIWorkList {
+    CallInst *CI;
----------------
CICandidate or CICand?

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:104
@@ +103,3 @@
+  };
+  void collectCandidates(CallInst &CI);
+  void generateOneRangeCond(CallInst &CI, const LibFunc::Func &Func);
----------------
-> checkCandidate

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:110
@@ +109,3 @@
+
+  Value *CreateCond(CallInst &CI, CmpInst::Predicate Cmp, float Val,
+                    CmpInst::Predicate Cmp2, float Val2) {
----------------
Missing documentation for the method. Also name it "CreateOrCond".

I also  think it is better to pass the Arg directly instead of the CallInst&

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:118
@@ +117,3 @@
+
+  Value *CreateCond(CallInst &CI, CmpInst::Predicate Cmp, float Val) {
+    IRBuilder<> BBBuilder(&CI);
----------------
Missing documentation; Pass Arg0 directly

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:127
@@ +126,3 @@
+
+  Value *CreateCondEQInf(CallInst &CI) {
+    IRBuilder<> BBBuilder(&CI);
----------------
This method can refactored to share code with CreateCond methods that takes two compares.

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:143
@@ +142,3 @@
+// Find all conditonal deal call DCE candicates and push to a worklist.
+void CondCallDCE::collectCandidates(CallInst &CI) {
+  if (CI.isNoBuiltin())
----------------
Fix documentation. This function does not 'find all ...'. It checks if CI is a candidate for shrinkwrapping and put it into work list if it is.

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:202
@@ +201,3 @@
+  case LibFunc::truncl: {
+    if (!CondCallDCEDoNoError)
+      return;
----------------
Perhaps break the giant switch into smaller helper functions. For instance define a helper function called: 

bool doesFuncSetErrNo(...) { ..
   }


================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:211
@@ +210,3 @@
+  case LibFunc::acosl: // Same as acos
+  case LibFunc::asin:  // DomeinError: (x < -1 || x > 1)
+  case LibFunc::asinf: // Same as asin
----------------
DomeinError -> DomainError

================
Comment at: lib/Transforms/Utils/CondDeadCallElimination.cpp:552
@@ +551,3 @@
+bool runImpl(Function &F, const TargetLibraryInfo &TLI) {
+  if (CondCallDCEDisable)
+    return false;
----------------
When OptimizeForSize is on, this pass should not be turned on.


https://reviews.llvm.org/D24414





More information about the llvm-commits mailing list