llvm dev - Jun 2017 - Ok with mismatch between dead-markings in BUNDLE and bundled instructions?

> On Jun 27, 2017, at 2:10 PM, Matthias Braun <mbraun at apple.com>
wrote:
> 
>> 
>> On Jun 27, 2017, at 4:55 AM, Mikael Holmén via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> 
>> Hi Quentin and llvm-dev,
>> 
>> I've got a regalloc-related question that you might have an opinion
or answer about.
>> 
>> In our out-of-tree target we've been doing some bundling before
register allocation for quite some time now, and last night a new problem popped
up. What the fix should be depends on if this bundle is legal or not:
>> 
>>   BUNDLE %vreg39<imp-def,dead>
>>     * %vreg39:hiAcc<def> = mv_ar16_ar16_lo16In32 %vreg37
>>     [...]
>> 
>> %vreg39 isn't used after the bundle so the dead-marking in the
BUNDLE is correct. However, the def in the actual bundled instruction defining
%vreg39 is not marked with dead.
>> 
>> Is the above bundle ok or not?
> 
> As to whether the bundle is ok: The register allocator should view the
whole bundle as a single instruction with all operands combined. That would
conceptually give us:
> 
>    %vreg39<imp-def, dead>, %vreg39:hiAcc<def> = INST...
> 
> Of course having two defs for the same register is an uncommon situation.
There's some interesting arguments to be had whether this example means the
whole vreg39 is dead or just the parts of vreg39 that are not covered by the
hiAcc subregister. I think in todays code it means that the whole vreg39
register is dead (though I cannot say off hand whether we do this consistently
or whether it is actually documented somewhere).
Yeah I was reading this as “only the non-touched part are dead”, and that’s what
I’d like to see in the representation longer. Obviously, the register is not
dead as a whole here :)
> 
>> 
>> 
>> When the register allocator later tries to spill/reload %vreg39 it
thinks that %vreg39 is live, so it inserts a reload after the bundle, but then
the verifier pukes on it and says that there is a use of the register after
it's dead.
>> 
>> The dead-marking on the BUNDLE is introduced by
LiveIntervals::computeDeadValues which does
>> 
>>     // This is a dead def. Make sure the instruction knows.
>>     MachineInstr *MI = getInstructionFromIndex(Def);
>>     assert(MI && "No instruction defining live
value");
>>     MI->addRegisterDead(LI.reg, TRI);
> 
> I think the actual problem here is that MachineInstr::addRegisterDead only
works on a single instruction and not on the whole bundle.
> 
> - Matthias
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170627/bf8ccedb/attachment-0001.html>

> On Jun 27, 2017, at 2:35 PM, Quentin Colombet via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
>> 
>> On Jun 27, 2017, at 2:10 PM, Matthias Braun <mbraun at apple.com
<mailto:mbraun at apple.com>> wrote:
>> 
>>> 
>>> On Jun 27, 2017, at 4:55 AM, Mikael Holmén via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>> 
>>> Hi Quentin and llvm-dev,
>>> 
>>> I've got a regalloc-related question that you might have an
opinion or answer about.
>>> 
>>> In our out-of-tree target we've been doing some bundling before
register allocation for quite some time now, and last night a new problem popped
up. What the fix should be depends on if this bundle is legal or not:
>>> 
>>>   BUNDLE %vreg39<imp-def,dead>
>>>     * %vreg39:hiAcc<def> = mv_ar16_ar16_lo16In32 %vreg37
>>>     [...]
>>> 
>>> %vreg39 isn't used after the bundle so the dead-marking in the
BUNDLE is correct. However, the def in the actual bundled instruction defining
%vreg39 is not marked with dead.
>>> 
>>> Is the above bundle ok or not?
>> 
>> As to whether the bundle is ok: The register allocator should view the
whole bundle as a single instruction with all operands combined. That would
conceptually give us:
>> 
>>    %vreg39<imp-def, dead>, %vreg39:hiAcc<def> = INST...
>> 
>> Of course having two defs for the same register is an uncommon
situation. There's some interesting arguments to be had whether this example
means the whole vreg39 is dead or just the parts of vreg39 that are not covered
by the hiAcc subregister. I think in todays code it means that the whole vreg39
register is dead (though I cannot say off hand whether we do this consistently
or whether it is actually documented somewhere).
> 
> Yeah I was reading this as “only the non-touched part are dead”, and that’s
what I’d like to see in the representation longer. Obviously, the register is
not dead as a whole here :)
> 
>> 
>>> 
>>> 
>>> When the register allocator later tries to spill/reload %vreg39 it
thinks that %vreg39 is live, so it inserts a reload after the bundle, but then
the verifier pukes on it and says that there is a use of the register after
it's dead.
>>> 
>>> The dead-marking on the BUNDLE is introduced by
LiveIntervals::computeDeadValues which does
>>> 
>>>     // This is a dead def. Make sure the instruction knows.
>>>     MachineInstr *MI = getInstructionFromIndex(Def);
>>>     assert(MI && "No instruction defining live
value");
>>>     MI->addRegisterDead(LI.reg, TRI);
>> 
>> I think the actual problem here is that MachineInstr::addRegisterDead
only works on a single instruction and not on the whole bundle.
And yes, fixing that is the most sensible short term thing to do.
>> 
>> - Matthias
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://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/20170627/345fec31/attachment-0001.html>

On 6/27/2017 4:35 PM, Quentin Colombet via llvm-dev
wrote:
> Yeah I was reading this as “only the non-touched part are dead”, and 
> that’s what I’d like to see in the representation longer. Obviously, the 
> register is not dead as a whole here :)
I think that having two defs for the same register, one dead and one not 
dead simply doesn't make sense. We already assume that a register is 
live if at least a part of it is live, so if it's "dead", it
should mean
that the whole thing is dead.

-Krzysztof

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

> On Jun 27, 2017, at 2:44 PM, Krzysztof Parzyszek via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> On 6/27/2017 4:35 PM, Quentin Colombet via llvm-dev wrote:
>> Yeah I was reading this as “only the non-touched part are dead”, and
that’s what I’d like to see in the representation longer. Obviously, the
register is not dead as a whole here :)
> 
> I think that having two defs for the same register, one dead and one not
dead simply doesn't make sense. We already assume that a register is live if
at least a part of it is live, so if it's "dead", it should mean
that the whole thing is dead.
The register is not dead, but the value is. I like to keep the SSA semantic.
Here it happens that these two values are linked together because of the “name”
of the virtual register, but they really don’t need to be bound.

Put simply I’d like the dead flag to report the status of the value hold by this
definition, not this register. In other words, dead means you can kill the
instruction if no other side effects exists.
> 
> -Krzysztof
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
by The Linux Foundation
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

> On Jun 27, 2017, at 2:44 PM, Krzysztof Parzyszek via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> On 6/27/2017 4:35 PM, Quentin Colombet via llvm-dev wrote:
>> Yeah I was reading this as “only the non-touched part are dead”, and
that’s what I’d like to see in the representation longer. Obviously, the
register is not dead as a whole here :)
> 
> I think that having two defs for the same register, one dead and one not
dead simply doesn't make sense. We already assume that a register is live if
at least a part of it is live, so if it's "dead", it should mean
that the whole thing is dead.
Without subregister I would agree. However with subregisters and aliases in play
you can express more situations. Like for example:

    %rax<dead>, %eax = ...

could mean the instruction writes the full rax register but we are only gonna
read eax later. That said I am not sure whether we actually need it, and if llvm
works that way today. Given how subtle all of this is there is also a high
danger that we won't get the bahviour consistent.

- Matthias

On 6/27/2017 4:44 PM, Krzysztof Parzyszek via llvm-dev
wrote:
> I think that having two defs for the same register
I missed that one of them has a subregister.  Nevermind.

-Krzysztof

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation