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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Thanks for the reviews!
I have replied to 2 comments inline while I work on addressing the other comments.



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:393
+            getSpecializationBonus(&FormalArg, ActualArg) - Cost;
+        if (ForceFunctionSpecialization || Gain > 0)
+          Worklist.push_back({F, &FormalArg, ActualArg, Gain});
----------------
snehasish wrote:
> nit: We could avoid calling getSpecializationBonus if ForceFunctionSpecialization is true.
Good point, that will help compile-times. We still need to set Gain though, but what I will do is initialise Gain to the maximum value with ~0 if specialisation is forced. 


================
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)
----------------
snehasish wrote:
> 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?
Okay, good point. I changed the behaviour how `MaxConstantsThreshold` is interpreted.

Before, when the number of candidates in the worklist exceeded `MaxConstantsThreshold`, no specialisation was done at all. I have changed that to specialise up to `MaxConstantsThreshold` candidates, which seems to me what you would expect from `MaxConstantsThreshold`. After sorting the candidates on the maximum gain, it was this code getting rid of the remaining candidates with the lower gains:

      // Truncate the worklist to 'MaxConstantsThreshold' candidates if                               
      // necessary.
      if (Worklist.size() > MaxConstantsThreshold) {                                                  
        LLVM_DEBUG(dbgs() << "FnSpecialization: number of constants exceed "                          
                          << "the maximum number of constants threshold.\n"                           
                          << "Truncating worklist to " << MaxConstantsThreshold                       
                          << " candidates.\n");
        Worklist.erase(Worklist.begin() + MaxConstantsThreshold,
                       Worklist.end());
      }

I included this change in behaviour here, because otherwise the sorting doesn't serve a purpose if we are going to keep all candidates anyway. But given that I am going to follow up on this, I will keep the sorting but move this change in behaviour in a follow up and dependent patch. 

To finally answer your question about this test change: there are 2 candidates here, but `-func-specialization-max-constants=1`. With the old behaviour, this means no specialisation at all like I mentioned before. With the new interpretation, this specialises up to 1 candidate, which is what you see here.



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

https://reviews.llvm.org/D115458