llvm dev - Feb 2019 - Dealing with illegal operand mappings in RegBankSelect

Hi,

Some operations on AMDGPU require operands which must be in a register bank that
is impossible to copy from another. The operation needs to be rewritten in a
complex way to avoid the illegal copy.

Currently if I correctly report the required register banks in the operand
mapping, RegBankSelect happily inserts the illegal copies, somehow concluding
they are cheap (I would at least hope there’s an assert or something). So far
I’ve worked around this by lying and reporting all of the invalid source
register banks as legal. applyMapping then checks these registers and creates
the new vregs and rewrites as necessary. Is this the way this is intended to
work? This sort of makes sense, since it is legal in the sense that applyMapping
can deal with it. The cases that need to be rewritten can then have a very high,
but not impossible cost in the case of getInstrAlternativeMappings.

Lying about the actually legal banks seems counterintuitive to me. Should there
be something like a new RepairingPlacement type for this to just create the
vregs without inserting the copy?
-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190221/70265ebd/attachment.html>

Hi Matt,
> On Feb 20, 2019, at 4:49 PM, Arsenault, Matthew via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> Hi,
>  
> Some operations on AMDGPU require operands which must be in a register bank
that is impossible to copy from another. The operation needs to be rewritten in
a complex way to avoid the illegal copy.
>  
> Currently if I correctly report the required register banks in the operand
mapping, RegBankSelect happily inserts the illegal copies, somehow concluding
they are cheap (I would at least hope there’s an assert or something).
I would have hoped that we end up with a huge cost.
The assumption is that you can copy to/from any pair of register bank, then the
idea is that the copy should be lowered in something that makes sense (e.g., a
pair of store/load) by the target.
I am guessing that we could directly ask the target what pseudo-copy should be
inserted instead of always adding plain copy.

Would that work for you?
> So far I’ve worked around this by lying and reporting all of the invalid
source register banks as legal. applyMapping then checks these registers and
creates the new vregs and rewrites as necessary. Is this the way this is
intended to work?
I am not sure I understand the work around.
Let me try to illustrate this with an example.
Let say we have:
= inst a(bank1)
And inst only accepts bank2.
Are you saying that you lie by saying that bank1 is legal in that case?
If so, why applyMapping would do any rewriting since this is legal.
> This sort of makes sense, since it is legal in the sense that applyMapping
can deal with it. The cases that need to be rewritten can then have a very high,
but not impossible cost in the case of getInstrAlternativeMappings.
>  
> Lying about the actually legal banks seems counterintuitive to me.
Agree, that’s not the way it’s supposed to work.
> Should there be something like a new RepairingPlacement type for this to
just create the vregs without inserting the copy?
See my previous comment on what the copies are supposed to mean. I guess we
could imagine a new kind of repairing placement but I am not sure what it would
bring us, in the sense of lowering the (pseudo) copy differently should already
do what we want.

Cheers,
-Quentin
> -Matt
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<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/20190220/5f3ae49d/attachment-0001.html>

> On Feb 21, 2019, at 12:18 AM, Quentin Colombet via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi Matt,
> 
>> On Feb 20, 2019, at 4:49 PM, Arsenault, Matthew via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>> 
>> Hi,
>>  
>> Some operations on AMDGPU require operands which must be in a register
bank that is impossible to copy from another. The operation needs to be
rewritten in a complex way to avoid the illegal copy.
>>  
>> Currently if I correctly report the required register banks in the
operand mapping, RegBankSelect happily inserts the illegal copies, somehow
concluding they are cheap (I would at least hope there’s an assert or
something).
> 
> I would have hoped that we end up with a huge cost.
> The assumption is that you can copy to/from any pair of register bank, then
the idea is that the copy should be lowered in something that makes sense (e.g.,
a pair of store/load) by the target.
> I am guessing that we could directly ask the target what pseudo-copy should
be inserted instead of always adding plain copy.
> 
> Would that work for you?
You can’t legitimately copy from vector to scalar. It conceptually doesn’t work,
and going through memory doesn’t help. The use instruction needs to be rewritten
to (in the worst case) scalarize the operation for every workitem. A pseudocopy
would still be some illegal operation which cannot exist which would need to be
guaranteed to be removed, so I don’t think this would be any cleaner than
allowing the illegal copies.

> 
>> So far I’ve worked around this by lying and reporting all of the
invalid source register banks as legal. applyMapping then checks these registers
and creates the new vregs and rewrites as necessary. Is this the way this is
intended to work?
> 
> I am not sure I understand the work around.
> Let me try to illustrate this with an example.
> Let say we have:
> = inst a(bank1)
> And inst only accepts bank2.
> Are you saying that you lie by saying that bank1 is legal in that case?
> If so, why applyMapping would do any rewriting since this is legal.
Yes. It’s what happens here: https://reviews.llvm.org/D58511
<https://reviews.llvm.org/D58511>

For extract_vector_elt, the indirect register index type must be an SGPR. I’ve
implemented the mapping as just copying the original source bank and allowing
applyMapping to deal with it. There might be cases that aren’t correctly being
rewritten, but I haven’t finished handling every case to run into them yet.
> 
>> This sort of makes sense, since it is legal in the sense that
applyMapping can deal with it. The cases that need to be rewritten can then have
a very high, but not impossible cost in the case of getInstrAlternativeMappings.
>>  
>> Lying about the actually legal banks seems counterintuitive to me.
> 
> Agree, that’s not the way it’s supposed to work.
> 
>> Should there be something like a new RepairingPlacement type for this
to just create the vregs without inserting the copy?
> 
> See my previous comment on what the copies are supposed to mean. I guess we
could imagine a new kind of repairing placement but I am not sure what it would
bring us, in the sense of lowering the (pseudo) copy differently should already
do what we want.
> 
It would avoid producing a totally illegal operation that something would need
to be responsible for ensuring is deleted

-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190226/b82c72c0/attachment.html>