llvm dev - Aug 2014 - [LLVMdev] Help with definition of subregisters; spill, rematerialization and implicit uses

Hi,

I have a problem regarding sub-register definitions and LiveIntervals on
our target. When a subregister is defined, other parts of the register
are always left untouched - they are neither read or def:ed.

It however seems that Codegen treats subregister definitions as somehow
clobbering the whole register.

The SSA-code looks like this after isel:

(Reg0 and Reg1 are 16bit registers. Reg2, Reg3 and Reg4 are 32 bit
registers with 16bit subregs, hi16 and lo16.)

Reg0 = #imm0
Reg1 = #imm1

Reg2 = IMPLICIT_DEF
Reg3 = INSERT_SUBREG Reg2, Reg0, hi16
Reg4 = INSERT_SUBREG Reg3, Reg1, lo16

After TwoAddressInstructionPass it becomes:

Reg5:hi16<def,read-undef> = Reg0
Reg5:lo16<def>            = Reg1

So, in my world this means a setting of the high 16 bits in Reg5 (not
affecting the low part) followed by a setting of the low 16 bits (not
affecting the high part). Is this how LLVM looks at it too?

(What does the "read-undef" part really mean, since in my
world, the setting of lo16 or hi16 does not in any way affect or
access the other part of the register.)

The question is: How should true subregister definitions be
expressed so that they do not interfere with each other? See the
detailed problem description below.

---

During RA it's decided Reg5 should be spilled and it's also decided Reg5
can be rematerialized:

"Value Reg5:0 at 5000r may remat from Reg5:hi16<def,read-undef> =
mv_any16
32766"

So it says Reg5 can be rematerialized by setting it's high part...

We also get:

      reload:   5052r    Reg5<def> = Load40FI <fi#2>
      rewrite: 5056r    Reg5:lo16<def> = mv_nimm6_ar16 0

So it inserts a reload of the full Reg5 prior to the setting of
Reg5:lo16, because it thinks there is an implicit use of Reg5 when
writing the low part??? This seems very weird to me.

The decision is based on the fact that MachineOperand::readsReg()
returns true:

    /// readsReg - Returns true if this operand reads the previous value
of its
    /// register.  A use operand with the <undef> flag set doesn't
read its
    /// register.  A sub-register def implicitly reads the other parts 
of the
    /// register being redefined unless the <undef> flag is set.
    ///
    /// This refers to reading the register value from before the current
    /// instruction or bundle. Internal bundle reads are not included.
    bool readsReg() const {
      assert(isReg() && "Wrong MachineOperand accessor");
      return !isUndef() && !isInternalRead() && (isUse() ||
getSubReg());
    }

I don't get why we automatically should get an implicit use just
because we are writing a subreg.

Since Reg5:lo16 is defined with

Reg5:lo16<def>            = Reg1

isUndef() will return false and getSubReg() true, and thus readsReg()
true and the reload is inserted.

Then we get

*** Bad machine code: Instruction loads from dead spill slot ***

because the spill slot has not been written.

Most grateful for any help,
Mikael Holmén

Hi Mikael,

On Aug 15, 2014, at 3:42 AM, Mikael Holmén <mikael.holmen at ericsson.com>
wrote:
> Hi,
> 
> I have a problem regarding sub-register definitions and LiveIntervals on
> our target. When a subregister is defined, other parts of the register
> are always left untouched - they are neither read or def:ed.
> 
> It however seems that Codegen treats subregister definitions as somehow
> clobbering the whole register.
> 
> The SSA-code looks like this after isel:
> 
> (Reg0 and Reg1 are 16bit registers. Reg2, Reg3 and Reg4 are 32 bit
> registers with 16bit subregs, hi16 and lo16.)
> 
> Reg0 = #imm0
> Reg1 = #imm1
> 
> Reg2 = IMPLICIT_DEF
> Reg3 = INSERT_SUBREG Reg2, Reg0, hi16
> Reg4 = INSERT_SUBREG Reg3, Reg1, lo16
> 
> After TwoAddressInstructionPass it becomes:
> 
> Reg5:hi16<def,read-undef> = Reg0
> Reg5:lo16<def>            = Reg1
> 
> So, in my world this means a setting of the high 16 bits in Reg5 (not
> affecting the low part) followed by a setting of the low 16 bits (not
> affecting the high part). Is this how LLVM looks at it too?
Yes, it is.
> 
> (What does the "read-undef" part really mean, since in my
> world, the setting of lo16 or hi16 does not in any way affect or
> access the other part of the register.)
read-undef means that we care only about the part we are defining.
Reg5:hi16<def,read-undef> means that whatever is set in Reg5, but hi16, we
do not care.
Reg5:lo16<def>  means that we do care about the value of Reg5:hi16.

Here is the exact definition:
  /// IsUndef - True if this register operand reads an "undef" value,
i.e. the
  /// read value doesn't matter.  This flag can be set on both use and def
  /// operands.  On a sub-register def operand, it refers to the part of the
  /// register that isn't written.  On a full-register def operand, it is a
  /// noop.  See readsReg().
> 
> The question is: How should true subregister definitions be
> expressed so that they do not interfere with each other? See the
> detailed problem description below.
We do have a limitation in our current liveness tracking for sub-register.
Therefore, I am not sure that is possible.

Conceptually, what you want is:
Reg5:hi16<def,read-undef>
Reg5:lo16<def,read-undef>

However, I guess this wouldn’t be supported, because it would mean that we do
not care about the value of hi16 at the definition of Reg5:lo16. This is true,
but after this definition we do care about hi16 and I am afraid read-undef does
not convey the right information for the subsequent uses of Reg5.

You can give it a try and see how it goes.
> 
> ---
> 
> During RA it's decided Reg5 should be spilled and it's also decided
Reg5
> can be rematerialized:
> 
> "Value Reg5:0 at 5000r may remat from Reg5:hi16<def,read-undef>
= mv_any16
> 32766"
> 
> So it says Reg5 can be rematerialized by setting it's high part...
> 
> We also get:
> 
>     reload:   5052r    Reg5<def> = Load40FI <fi#2>
>     rewrite: 5056r    Reg5:lo16<def> = mv_nimm6_ar16 0
> 
> So it inserts a reload of the full Reg5 prior to the setting of
> Reg5:lo16, because it thinks there is an implicit use of Reg5 when
> writing the low part??? This seems very weird to me.
> 
> The decision is based on the fact that MachineOperand::readsReg()
> returns true:
> 
>   /// readsReg - Returns true if this operand reads the previous value
> of its
>   /// register.  A use operand with the <undef> flag set doesn't
read its
>   /// register.  A sub-register def implicitly reads the other parts of the
>   /// register being redefined unless the <undef> flag is set.
>   ///
>   /// This refers to reading the register value from before the current
>   /// instruction or bundle. Internal bundle reads are not included.
>   bool readsReg() const {
>     assert(isReg() && "Wrong MachineOperand accessor");
>     return !isUndef() && !isInternalRead() && (isUse() ||
getSubReg());
>   }
> 
> I don't get why we automatically should get an implicit use just
> because we are writing a subreg.
> 
> Since Reg5:lo16 is defined with
> 
> Reg5:lo16<def>            = Reg1
> 
> isUndef() will return false and getSubReg() true, and thus readsReg()
> true and the reload is inserted.
> 
> Then we get
> 
> *** Bad machine code: Instruction loads from dead spill slot ***
> 
> because the spill slot has not been written.
This is weird. Any chance you could share a test case?

Thanks,
-Quentin
> 
> Most grateful for any help,
> Mikael Holmén
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140815/069a1fbb/attachment.html>

Hi Quentin,

On 08/15/14 19:01, Quentin Colombet wrote:
[...]
>> The question is: How should true subregister definitions be
>> expressed so that they do not interfere with each other? See the
>> detailed problem description below.
>
> We do have a limitation in our current liveness tracking for
> sub-register. Therefore, I am not sure that is possible.
>
> Conceptually, what you want is:
> Reg5:hi16<def,read-undef>
> Reg5:lo16<def,read-undef>
>
> However, I guess this wouldn’t be supported, because it would mean that
> we do not care about the value of hi16 at the definition of Reg5:lo16.
> This is true, but after this definition we do care about hi16 and I am
> afraid read-undef does not convey the right information for the
> subsequent uses of Reg5.
>
> You can give it a try and see how it goes.
I tried setting isUndef to trie when handling INSERT_SUBREG in 
TwoAddressInstructioPass.cpp, but then I run into stuff like this instead:

832B            %vreg50:hi16<def,read-undef> = COPY %vreg0
848B            ...
864B            %vreg19<def,dead> = COPY %vreg50
880B            %vreg19:lo16<def,read-undef> = COPY %vreg73
896B            ...
912B            mv_a32_r16_rmod1 %vreg19, %vreg20

...

*** Bad machine code: Multiple connected components in live interval ***
- function:    fixedconv
- interval:    %vreg19 [864r,864d:0)[880r,1024r:1)  0 at 864r 1 at 880r
0: valnos 0
1: valnos 1

So here, both the setting of the hi16 and lo16 parts are marked with 
read-undef, as wanted. However

864B            %vreg19<def,dead> = COPY %vreg50

looks suspicious to me. It's like it thinks that

880B            %vreg19:lo16<def,read-undef> = COPY %vreg73

is redefining the whole vreg19, not only the lo16 part, and since this 
instruction has read-undef, it thinks no part of vreg19, not even hi16 
is live over instruction 880.
>>
>> isUndef() will return false and getSubReg() true, and thus readsReg()
>> true and the reload is inserted.
>>
>> Then we get
>>
>> *** Bad machine code: Instruction loads from dead spill slot ***
>>
>> because the spill slot has not been written.
>
> This is weird. Any chance you could share a test case?
Unfortunately not. I'm running our out-of-tree backend and I've no idea 
if anything like this ever happens in other backends :(

Thanks!
/Mikael