llvm dev - Nov 2019 - InlineSpiller - hoists leave virtual registers without live intervals

/// Optimizations after all the reg selections and spills are done.
	void InlineSpiller::postOptimization() { HSpiller.hoistAllSpills();
}

Seems a problematic function to me, as hoistAllSpills() uses
TII.storeRegToStackSlot() to insert new spills.

The problem is, TII.storeRegToStackSlot is allowed to create new virtual
registers, which can not be allocated a range as this whole thing is called
_after_ all reg selection is complete.

If I'm right in this, I do not see how the in-tree target AMDGPU::SI has not
been affected, as it creates virtual registers in both load and store stack
operations in SIInstrInfo.cpp - which is where I confirmed to myself that it
was okay to do so. When compilation broke,
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130812/184331.htm
l further suggested that the intention is that you can... but I do not see
how a hoist can ever pass verification/compile correctly. Am I doing
something incorrectly here? Or are hoistable spills just that rare on GPU
code that it's never come up?

As a side note, compiler option "-disable-spill-hoist"
(DisableHoisting) is
referenced in exactly zero places, so if anyone in a similar situation finds
this post, maybe don't bother testing with that flag just yet. :)

Hi Alex,

Thanks for reporting this.
Wei worked on the hoisting optimization.

@Wei, could you work with Alex to see what is the problem.

Cheers,
-Quentin
> On Nov 3, 2019, at 5:20 AM, via llvm-dev <llvm-dev at lists.llvm.org>
wrote:
> 
> 	/// Optimizations after all the reg selections and spills are done.
> 	void InlineSpiller::postOptimization() { HSpiller.hoistAllSpills();
> }
> 
> Seems a problematic function to me, as hoistAllSpills() uses
> TII.storeRegToStackSlot() to insert new spills.
> 
> The problem is, TII.storeRegToStackSlot is allowed to create new virtual
> registers, which can not be allocated a range as this whole thing is called
> _after_ all reg selection is complete.
> 
> If I'm right in this, I do not see how the in-tree target AMDGPU::SI
has not
> been affected, as it creates virtual registers in both load and store stack
> operations in SIInstrInfo.cpp - which is where I confirmed to myself that
it
> was okay to do so. When compilation broke,
>
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130812/184331.htm
> l further suggested that the intention is that you can... but I do not see
> how a hoist can ever pass verification/compile correctly. Am I doing
> something incorrectly here? Or are hoistable spills just that rare on GPU
> code that it's never come up?
> 
> As a side note, compiler option "-disable-spill-hoist"
(DisableHoisting) is
> referenced in exactly zero places, so if anyone in a similar situation
finds
> this post, maybe don't bother testing with that flag just yet. :)
> 	
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Mon, Nov 4, 2019 at 12:18 PM Quentin Colombet <qcolombet at apple.com>
wrote:
> Hi Alex,
>
> Thanks for reporting this.
> Wei worked on the hoisting optimization.
>
> @Wei, could you work with Alex to see what is the problem.
>
> Cheers,
> -Quentin
>
> > On Nov 3, 2019, at 5:20 AM, via llvm-dev <llvm-dev at
lists.llvm.org>
> wrote:
> >
> >       /// Optimizations after all the reg selections and spills are
done.
> >       void InlineSpiller::postOptimization() {
HSpiller.hoistAllSpills();
> > }
> >
> > Seems a problematic function to me, as hoistAllSpills() uses
> > TII.storeRegToStackSlot() to insert new spills.
> >
> > The problem is, TII.storeRegToStackSlot is allowed to create new
virtual
> > registers, which can not be allocated a range as this whole thing is
> called
> > _after_ all reg selection is complete.
> >
> > If I'm right in this, I do not see how the in-tree target
AMDGPU::SI has
> not
> > been affected, as it creates virtual registers in both load and store
> stack
> > operations in SIInstrInfo.cpp - which is where I confirmed to myself
> that it
> > was okay to do so. When compilation broke,
> >
>
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130812/184331.htm
> > l further suggested that the intention is that you can... but I do not
> see
> > how a hoist can ever pass verification/compile correctly. Am I doing
> > something incorrectly here? Or are hoistable spills just that rare on
GPU
> > code that it's never come up?
>
Hi Alex,

I havn't noticed before that TII.storeRegToStackSlot is allowed to create
new virtual registers. I am surprised to know it is allowed to do that. I
am mostly working on x86 and TII.storeRegToStackSlot for x86 doesn't create
new register. As you said, if TII.storeRegToStackSlot is allowed to create
new virtual registers, that could be a problem. It is not exposed maybe
because of some later pass cover the problem -- maybe RegScavenger?

Are you aware of any other architecture other than AMDGPU on
which TII.storeRegToStackSlot creates new virtual registers? Could you
explain in which case new virtual registers will be created?

Thanks,
Wei.

> >
> > As a side note, compiler option "-disable-spill-hoist"
(DisableHoisting)
> is
> > referenced in exactly zero places, so if anyone in a similar situation
> finds
> > this post, maybe don't bother testing with that flag just yet. :)
> >
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191104/58990152/attachment.html>