[PATCH] D86864: [MachineSinking] sink more profitable loads

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

In D86864#2472387 <https://reviews.llvm.org/D86864#2472387>, @LiuChen3 wrote:

> In D86864#2472375 <https://reviews.llvm.org/D86864#2472375>, @shchenz wrote:
>
>> In D86864#2472369 <https://reviews.llvm.org/D86864#2472369>, @LiuChen3 wrote:
>>
>>>> I can surely do that.  But I think the most reasonable solution would be fix the compiling time issue.  Since compiling time tests I did before does not expose any regression, your test case must be a little special.  Could you find out the special point, for example the function has too many blocks or some/many blocks in the function has too many instructions?  Thanks.
>>>
>>> I think the increase in compile time is because the function has too many instructions and blocks. The function has Thousands of lines of instructions. Can you add limitation for the number of instructions or the number of blocks so the check for 'store' can end early?
>>
>> Could you please help to check if the following change can solve your issue?
>>
>>   C
>>   diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
>>   index 0abdf89..8ca3520 100644
>>   --- a/llvm/lib/CodeGen/MachineSink.cpp
>>   +++ b/llvm/lib/CodeGen/MachineSink.cpp
>>   @@ -79,6 +79,12 @@ static cl::opt<unsigned> SplitEdgeProbabilityThreshold(
>>            "splitted critical edge"),
>>        cl::init(40), cl::Hidden);
>>    
>>   +static cl::opt<unsigned> SinkLoadInstsPerBlockThreshold(
>>   +    "machine-sink-load-instrs-threshold",
>>   +    cl::desc("Do not try to find alias store for a load if there is a in-path "
>>   +             "block whose instruction number is higher than this threshold."),
>>   +    cl::init(2000), cl::Hidden);
>>   +
>>    STATISTIC(NumSunk,      "Number of machine instructions sunk");
>>    STATISTIC(NumSplit,     "Number of critical edges split");
>>    STATISTIC(NumCoalesces, "Number of copies coalesced");
>>   @@ -1036,6 +1042,12 @@ bool MachineSinking::hasStoreBetween(MachineBasicBlock *From,
>>        HandledBlocks.insert(BB);
>>        // To post dominates BB, it must be a path from block From.
>>        if (PDT->dominates(To, BB)) {
>>   +      // If this BB is too big, stop searching to save compiling time.
>>   +      if (BB->size() > SinkLoadInstsPerBlockThreshold) {
>>   +        HasStoreCache[BlockPair] = true;
>>   +        return true;
>>   +      }
>>   +
>>          for (MachineInstr &I : *BB) {
>>            // Treat as alias conservatively for a call or an ordered memory
>>            // operation.
>
> Unfortunately, this patch doesn't work. I don't think the number of blocks is necessarily related to the number of instructions.
> For one function, with your default threshold, The time spent on Machine code sinking is 11.2464s, no obvious difference from before. When set the threshold to 1, the time spent on Machine code sinking reduced 0.6937.
> I think it would be better if you can limit the number of MIs.
> Anyway, this patch provides us with an option. Thanks for your help.

The above threshold is for number of MIs. `BB->size()` is to get instruction number of BB. I committed `31c2b93d83f63ce7f9bb4977f58de2e00bf18e0f` to further reduce compiling time. You can have a try


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86864