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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish accepted this revision.
snehasish added a comment.

lgtm



================
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)
----------------
SjoerdMeijer wrote:
> 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.
> 
Thanks for clarifying the change.


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

https://reviews.llvm.org/D115458