One GlobalISel compile time optimization patch (https://reviews.llvm.org/D109750) has generated some debate over whether it’s semantically allowed/not-an-error for a DBG_VALUE machine instruction to have a use of a vreg that doesn’t have a corresponding definition.
We talked internally with Adrian and Vedant and didn’t come to a strong conclusion either way. Does anyone have thoughts here?
Thanks,
Amara
I can't really be speak about debug instructions in particular. For machine instructions in general you just mark the use operand with "undef" and you are good...
- Matthias
That’s what we were doing in the legalizer, but calling eraseFromParentAndMarkDBGValuesForRemoval() is much more expensive than a plain eraseFromParent(), so the motivating change was to use a heuristic to avoid calling it when it was very likely that it wasn’t going to have a debug user.
Whichever answer we get is fine, we can have a cleanup phase after legalization over all vregs in MachineRegisterInfo to mark ones without defs as undef, but I’d like to avoid doing that if we can so we don’t spend the extra compile time.
Amara
Hi,
One GlobalISel compile time optimization patch (⚙ D109750 [GlobalISel][Legalizer] Don't use eraseFromParentAndMarkDBGValuesForRemoval() for some artifacts.) has generated some debate over whether it’s semantically allowed/not-an-error for a DBG_VALUE machine instruction to have a use of a vreg that doesn’t have a corresponding definition.
I don't think there's a semantic reason why DBG_VALUEs of undefined
vregs should be disallowed -- they can be interpreted the same as a
DBG_VALUE $noreg, just in a non-canonical form. They would correctly
become DBG_VALUE $noreg's when LiveDebugVariables runs, as the vreg
wouldn't be live.
In practical terms, it could slightly mislead people reading MIR, and
there might be code out there that expects to find a vreg definition
for anything that DBG_VALUEs refer to, which I imagine would be fairly
easy to work around.
So you are asking about whether
... no definition of %X anywhere ...
DBG_VALUE %x ; No undef flag here
is fine. I think technically nothing will stop you. DBG_VALUE instructions are not real uses and ignored for anything semantically interesting like liveness etc. Thinking about it from this angle it wouldn't be that far fetched to just mark all DBG_VALUE operands as `undef`.
On the other hand motivating this with `eraseFromParentAndMarkDBGValuesForRemoval` vs. `eraseFromParent` feels surprising... Somehow I would have assumed that once you finished all the work of creating a DBG_VALUE instruction and a register and multiple passes reading those instructions and operands, that one more iteration over the operands wouldn't really make a noticeable performance difference...
- Matthias
So you are asking about whether
... no definition of %X anywhere ...
DBG_VALUE %x ; No undef flag hereis fine. I think technically nothing will stop you. DBG_VALUE instructions are not real uses and ignored for anything semantically interesting like liveness etc. Thinking about it from this angle it wouldn't be that far fetched to just mark all DBG_VALUE operands as `undef`.
On the other hand motivating this with `eraseFromParentAndMarkDBGValuesForRemoval` vs. `eraseFromParent` feels surprising... Somehow I would have assumed that once you finished all the work of creating a DBG_VALUE instruction and a register and multiple passes reading those instructions and operands, that one more iteration over the operands wouldn't really make a noticeable performance difference…
The move to use eraseFromParent() was in a part of the legalizer that can see very high numbers of artifact instructions being created and deleted as they’re combined away, so this region of code can be hot on a profile.