[PATCH] D115458: [FuncSpec] Decouple Cost and Benefit analysis, to sort candidates.

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:98
+struct ArgInfo {
+  Function *Fn;         // The function to perform specialisaiton on.
+  Argument *Arg;        // The Formal argument being analysed.
----------------
typo "specialization"


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:393
+            getSpecializationBonus(&FormalArg, ActualArg) - Cost;
+        if (ForceFunctionSpecialization || Gain > 0)
+          Worklist.push_back({F, &FormalArg, ActualArg, Gain});
----------------
nit: We could avoid calling getSpecializationBonus if ForceFunctionSpecialization is true.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:401
+      // Sort the candidates in descending order.
+      std::sort(Worklist.begin(), Worklist.end(), [] (ArgInfo L, ArgInfo R) {
+                                          return L.Gain > R.Gain; } );
----------------
Use a reference to avoid copies here?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:430
+      // FIXME: Only one argument per function.
+      return Worklist;
+    }
----------------
nit: just break here instead of return?


================
Comment at: llvm/test/Transforms/FunctionSpecialization/function-specialization4.ll:41
 
-; CONST1-NOT: define internal i32 @foo.1(i32 %x, i32* %b, i32* %c)
+; CONST1:     define internal i32 @foo.1(i32 %x, i32* %b, i32* %c)
 ; CONST1-NOT: define internal i32 @foo.2(i32 %x, i32* %b, i32* %c)
----------------
It wasn't clear to me what changed in the code that this function is now specialized. Is it possible to split this into a refactor without functionality change and a smaller patch with the behaviour change?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115458/new/

https://reviews.llvm.org/D115458