llvm dev - Mar 2016 - PHI node to different register class vs TailDuplication

Hi,

We're having an issue with TailDuplication in our out-of-tree target and 
it's this PHI-node that seems to be the cause of the trouble:

	%vreg2<def> = PHI %vreg0, <BB#2>, %vreg1, <BB#3>; rN:%vreg2 
aNlh_0_7:%vreg0 aNlh_rN:%vreg1

Note that the defined %vreg2 has register class "rN" while the read 
%vreg0 has register class "aNlh_0_7". "rN" and
"aNlh_0_7" are disjoint.

Is such a PHI node ok?

If it is, then there is a bug in TailDuplication.

Before TailDuplication we have:

BB#2: derived from LLVM BB %bb2
     Predecessors according to CFG: BB#1
	%vreg12<def> = mv16Sym <ga:@a>; rN:%vreg12
	%vreg13<def> = mv_nimm6_ar16 0; aNlh_rN:%vreg13
	mv_ar16_r16_rmod1 %vreg13<kill>, %vreg12<kill>; aNlh_rN:%vreg13
rN:%vreg12
	brr_uncond <BB#4>;
     Successors according to CFG: BB#4(?%)

BB#4: derived from LLVM BB %bb4
     Predecessors according to CFG: BB#2 BB#3
	%vreg2<def> = PHI %vreg0, <BB#2>, %vreg1, <BB#3>; rN:%vreg2 
aNlh_0_7:%vreg0 aNlh_rN:%vreg1
	mv_a32_r16_rmod1 %vreg3, %vreg2; aN32_0_7:%vreg3 rN:%vreg2
	brr_uncond <BB#6>;
     Successors according to CFG: BB#6(?%)


Then TailDuplication runs

Tail-duplicating into PredBB: BB#2: derived from LLVM BB %bb2
[...]
 From Succ: BB#4: derived from LLVM BB %bb4

and we get:

BB#2: derived from LLVM BB %bb2
     Predecessors according to CFG: BB#1
	%vreg12<def> = mv16Sym <ga:@a>; rN:%vreg12
	%vreg13<def> = mv_nimm6_ar16 0; aNlh_rN:%vreg13
	mv_ar16_r16_rmod1 %vreg13<kill>, %vreg12<kill>; aNlh_rN:%vreg13
rN:%vreg12
	mv_a32_r16_rmod1 %vreg3, %vreg0; aN32_0_7:%vreg3 aNlh_0_7:%vreg0
	%vreg18<def> = COPY %vreg0; rN:%vreg18 aNlh_0_7:%vreg0
	brr_uncond <BB#6>;
     Successors according to CFG: BB#6(0x80000000 / 0x80000000 = 100.00%)

The problem here is the duplicated instruction

	mv_a32_r16_rmod1 %vreg3, %vreg0; aN32_0_7:%vreg3 aNlh_0_7:%vreg0

since %vreg0 has register class "aNlh_0_7" but the instruction expects
a
disjoint register class, "rN".

During duplication TailDuplication copied the instruction and then 
simply replaced %vreg2 with %vreg0 since they are connected through a 
PHI, but since the register classes differ, the resulting code is wrong.

I've managed to get around this by inserting a COPY in TailDuplication 
but I don't know what the proper fix is to this.

In TailDuplicatePass::ProcessPHI:

    const TargetRegisterClass *RC = MRI->getRegClass(DefReg);
+  const TargetRegisterClass *SrcRC = MRI->getRegClass(SrcReg);
+
+  // If the register class of the PHI src is wider than the PHI def
+  // then we can't just use PHI src instead of PHI def in the cloned
+  // instruction. Instead we insert a copy, copying the PHI src to a
+  // register of the wanted register class.
+  if (!RC->hasSubClassEq(SrcRC)) {
+    unsigned NewDef = MRI->createVirtualRegister(RC);
+
+    BuildMI(*PredBB, PredBB->instr_end(), DebugLoc(),
+            TII->get(TargetOpcode::COPY), NewDef).addReg(SrcReg);
+    SrcReg = NewDef;
+  }
+
    LocalVRMap.insert(std::make_pair(DefReg, SrcReg));

Any thoughts on this?
/Mikael

On 3/4/2016 6:44 AM, Mikael Holmén via llvm-dev wrote:
>
> Is such a PHI node ok?
That doesn't seem to be well documented. The PHI nodes are described as 
"representing an assignment", but what the "assignment"
could be is not
clear.  Even if there exists an instruction that could copy registers 
between register classes, if you cannot substitute the output operand 
with one of the input operands in any use of the output, the PHI would 
seem to be malformed.  At least, that is my interpretation.

-Krzysztof

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

A PHI can be thought of - and will be lowered to - COPY instructions at the end
of the respective predecessor blocks. So what you need in the example is that a
COPY instruction from class "aNlh_0_7" to class "rN" is
valid and that a COPY from class "aNlh_rN" to class "rN" is
valid.

- Matthias
> On Mar 4, 2016, at 11:09 AM, Krzysztof Parzyszek via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> On 3/4/2016 6:44 AM, Mikael Holmén via llvm-dev wrote:
>> 
>> Is such a PHI node ok?
> 
> That doesn't seem to be well documented. The PHI nodes are described as
"representing an assignment", but what the "assignment"
could be is not clear.  Even if there exists an instruction that could copy
registers between register classes, if you cannot substitute the output operand
with one of the input operands in any use of the output, the PHI would seem to
be malformed.  At least, that is my interpretation.
> 
> -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