This is an archive of the discontinued LLVM Phabricator instance.

[globalisel][tablegen] Require that all registers between instructions of a match are virtual.
ClosedPublic

Authored by dsanders on May 4 2017, 9:57 AM.

Details

Summary

Without this, it's possible to encounter multiple defs for a register.

This is triggered by the current version of D32868 when applied to trunk.

Event Timeline

dsanders created this revision.May 4 2017, 9:57 AM
qcolombet edited edge metadata.May 4 2017, 3:55 PM

If that happens, doesn't that means we need to support it?

The way I was thinking about this was that we should be able to insert copy to vreg before and after to assign to and from the physical registers.

If that happens, doesn't that means we need to support it?

The way I was thinking about this was that we should be able to insert copy to vreg before and after to assign to and from the physical registers.

As far as I know, it can't occur in SelectionDAG patterns. The closest we get is COPY_TO_REGCLASS which is still a vreg.

I also don't think we need it for GlobalISel-specific rules for ISel. This code is intended to detect whether there's an instruction defining the operand we're trying to follow to the def. Physical registers typically occur at the calling convention and inline assembly boundaries and as such there's nothing on the other side of them for us to match in a rule. In the longer term, we'll need to rework it to be able to re-use tablegen after the register allocator but I think it's best to leave that until we've finished ISel.

Hi Daniel,

We are talking pass each other :).

I think I miss the use case you want to solve.

On my side, I thought you wanted to fallback on cases where we have a physical register use or def like in ADJUSTSTACKxxx.

let Defs = [SP], Uses = [SP], [...] // <-- I am talking about that
[...]
def ADJCALLSTACKDOWN : [...]
[...]

Whereas I was saying we want to handle such case like this:
SP = vregIn
SP = ADJCALL SP
vregOut = SP

instead of falling back, given it will happen.

Cheers,
-Quentin

Hi Daniel,

We are talking pass each other :).

:-) For my part, I was trying to solve a problem that originates from tablegen matching the shape before testing the predicates.

For example, suppose you have a rule that matches (G_FADD (G_FMUL x, y), z). If the tablegen code is presented with:

%R0 = COPY %0
%R0 = COPY %1
%0 = COPY %R0  // <-- MI

it will capture the instructions for that rule with code to the effect of:

MI0 = MI;
MI1 = getVRegDef(MI0.getOperand(1).getReg());

triggering the assertion in getVRegDef() when multiple defs are detected.

I believe we can fix this for SelectionDAG-imported rules by checking the opcode as we go, but I think the problem will re-occur once we support GlobalISel specific rules. Particularly if targets introduce target-instructions before ISel. Forbidding physical registers covers both situations.

While I'm on the subject of issues involving getVRegDef(). The use of getVRegDef() is a bit questionable for another reason too. Nothing ensures that the def it finds occurs before the use. That's fine for vregs but that assumption isn't valid when you have something like:

liveins: %r0
%0 = COPY %r0 // getVRegDef(%r0) will return the instruction below
%r0 = COPY %0

which can occur during ISel.

I think I miss the use case you want to solve.

On my side, I thought you wanted to fallback on cases where we have a physical register use or def like in ADJUSTSTACKxxx.

let Defs = [SP], Uses = [SP], [...] // <-- I am talking about that
[...]
def ADJCALLSTACKDOWN : [...]
[...]

Whereas I was saying we want to handle such case like this:
SP = vregIn
SP = ADJCALL SP
vregOut = SP

instead of falling back, given it will happen.

Cheers,
-Quentin

I don't think we have the same problem on implicit uses/defs. Attempting to follow the def use chain with getVRegDef() will have the same problems as above, but tablegen will never try to do this because it only follows the chains for explicit uses.

qcolombet accepted this revision.May 11 2017, 5:55 PM

Thanks for the clarification, now I get it :).

The proposed approach sounds good. I was think that this will prevent us form having patterns with target specific opcodes (and thus implicit-def/use) but we can revisit if/when we get to that.

This revision is now accepted and ready to land.May 11 2017, 5:55 PM
dsanders closed this revision.May 17 2017, 5:56 AM