[PATCH] D137813: [RegAlloc Greedy]Account statepoints while splitting single basic block

[Serguei Katkov via Phabricator via llvm-commits]

skatkov marked an inline comment as not done.
skatkov added inline comments.


================
Comment at: llvm/lib/CodeGen/SplitKit.cpp:1622
+                                     CurLI->reg()))
+    return false;
   // Splitting a live-through range always makes progress.
----------------
skatkov wrote:
> qcolombet wrote:
> > That's awfully specific for this part of the code.
> > 
> > Could this be handled when rewriting the spills instead with `foldMemoryOperand`?
> Hi Quentin, first of all thank a lot for a review and you comment (to Matt as well) but now I feel I need a help.
> It looks like I misunderstand the reason why we split single basic block for single use at all.
> 
> The commit it was introduced has been done in 2011 with the following comment (and unfortunately without a test):
> commit 8627ea91cb097f45894fa80fa04a3fb0100530db
> Author: Jakob Stoklund Olesen <stoklund at 2pi.dk>
> Date:   Fri Aug 5 22:20:45 2011 +0000
> 
>     Split around single instructions to enable register class inflation.
> 
>     Normally, we don't create a live range for a single instruction in a
>     basic block, the spiller does that anyway. However, when splitting a
>     live range that belongs to a proper register sub-class, inserting these
>     extra COPY instructions completely remove the constraints from the
>     remainder interval, and it may be allocated from the larger super-class.
> 
>     The spiller will mop up these small live ranges if we end up spilling
>     anyway. It calls them snippets.
> 
> Split Single basic block is triggered by Greedy Register Allocator in two places:
> 1) Region split heuristic decided that we enter the current basic block on stack and exit it as well (if (!IntvIn && !IntvOut)).
> 2) Region split failed and we try to split each basic block where there are uses.
> 
> In both cases it is expected that enter basic block on stack and exit it on stack as well (if there are liveIn and liveOut respectively).
> I understand why we try to split if there are several uses. But if we have on use, we will do unspill/spill operation anyway why not to rely on InlineSpiller to create such new live intervals?
> 
> The comment above said that in this case remainder interval (I guess original interval without this one split out) removes the constrains and can be allocated from larger super-class. But Region splitter already isolated this basic block and remainder interval is already decided to be allocated on some register. Why do we need extra split here?
> I guess it is related to sub registers again and it makes me thinking that I misunderstand here something.
> 
> The trouble with your proposal to remove this line of the code and rely on InlineSpiller is as follows:
> The bad scenario looks as follows: we keep the live range for single use which is statepoint.
> There is no register pressure on this basic block at the end. As a result this interval will be allocated to some register.
> As a result in basic block we will have a unspill, use in statepoint and probably spill after statepoint.
> While we could avoid unspill/spill operation in this case at all due to statepoint is ok to accept the value on stack with the same efficiency as it was on register. But in this scenario InlineSpiller will not be triggered at all due to live interval will just be assigned to physical register.
> May be I miss something again :(
> 
> Ok, I go to learn again about sub-registers/spills and so on because this code makes me thinking I miss something.
> Any help would be very useful.
BTW: I disabled split for single instruction and got two test failures.
  LLVM :: CodeGen/AMDGPU/spill-vgpr.ll
  LLVM :: CodeGen/X86/fshl.ll

For x86, to me, the code becomes better.

For AMDGPU - not sure :)


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

https://reviews.llvm.org/D137813