Page MenuHomePhabricator

GlobalISel: Disallow undef generic virtual register uses
ClosedPublic

Authored by arsenm on Jun 29 2020, 6:45 AM.

Details

Summary

With an undef operand, it's possible for getVRegDef to fail and return
null. This is an edge case very little code bothered to
consider. Proper gMIR should use G_IMPLICIT_DEF instead.

I initially tried to apply this restriction to all SSA MIR, so then
getVRegDef would never fail anywhere. However, ProcessImplicitDefs
does technically run while the function is in SSA. ProcessImplicitDefs
and DetectDeadLanes would need to either move, or a new pseudo-SSA
type of function property would need to be introduced.

Diff Detail

Event Timeline

arsenm created this revision.Jun 29 2020, 6:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 6:45 AM
arsenm updated this revision to Diff 274093.Jun 29 2020, 6:46 AM

Remove leftover test change

This revision is now accepted and ready to land.Jun 29 2020, 10:35 AM
dsanders added subscribers: vsk, aprantl.

@vsk + @aprantl: I believe this is going to need an exception for DBG_VALUE. Could you confirm if that's correct?

My MIR is lacking here: What does it mean for a USE to be UNDEF? DBG_VALUE accepts UNDEF as an operand, but it's not clear to me what an undef vreg means / how it can be created.

My MIR is lacking here: What does it mean for a USE to be UNDEF? DBG_VALUE accepts UNDEF as an operand, but it's not clear to me what an undef vreg means / how it can be created.

You can set the undef flag on the MachineOperand.

ProcessImplicitDefs turns

%0 = IMPLICIT_DEF
FOO %0

into

FOO undef %0

You can also hand write this, but I don't think any pass before ProcessImplicitDefs and co should be using these

From that it *sounds* like a DBG_VALUE of an undef vreg doesn't make sense?

My MIR is lacking here: What does it mean for a USE to be UNDEF? DBG_VALUE accepts UNDEF as an operand, but it's not clear to me what an undef vreg means / how it can be created.

You can set the undef flag on the MachineOperand.

ProcessImplicitDefs turns

%0 = IMPLICIT_DEF
FOO %0

into

FOO undef %0

You can also hand write this, but I don't think any pass before ProcessImplicitDefs and co should be using these

Ah ok. This isn't quite what I thought it was when I quickly looked through earlier. The case I was worried about was when DBG_VALUE loses track of a variable and uses $noreg to indicate the register+value is undefined. Whereas this patch is concerned with a known vreg whose contents is undefined and lacks a defining instruction saying that.

From that it *sounds* like a DBG_VALUE of an undef vreg doesn't make sense?

I suppose even if it can happen then as far as DBG_VALUE is concerned it would be a known location (because it's not so concerned with the actual value as where the value is) and so applying the same rules makes sense.

arsenm marked an inline comment as done.Jun 30 2020, 4:11 PM
arsenm added inline comments.
llvm/lib/CodeGen/MachineVerifier.cpp
1704

The isSSA check here is a leftover and irrelevant for generic vregs