This was handled previously for arguments split due to not fitting in
an MVT. This was dropping the register for argument registers split
due to TLI::getRegisterTypeForCallingConv.
Details
- Reviewers
aprantl echristo scott.linder jmorse - Group Reviewers
debug-info
Diff Detail
Event Timeline
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
---|---|---|
5334 | for-range on N.op_values()? | |
5480 | I'm struggling to understand why values in the ValueMap need separate handling. IOW I don't get why it's not possible to handle the 'ArgRegsAndSizes.size() > 1' case earlier and assert '!RFV.occupiesMultipleRegs()' here. |
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
---|---|---|
5480 | RegsForValue expects a virtual register to exist already, and defined by some sequence somewhere. There's no corresponding live in register to the super-register in this case |
I think this looks reasonable, but am not familiar enough with the code to approve.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
---|---|---|
5480 | Thanks for explaining. I wonder if this could be simplified by sinking the ValueMap lookup into getUnderlyingArgRegs? |
This looks good to me, and I'm sort of familiar enough with the debug bits to approve. I've one concern about the change to getUnderlyingArgRegs though: previously it would fail-safe and return 0/$noreg if it encountered a node it didn't recognise. However now, if there's anything unrecognised in the middle of a vector/pair, the element will be silently skipped. That could lead to splitMultiRegDbgValue producing DBG_VALUEs with the wrong fragment offset, as it's unaware of the skipped element.
(I'm not aware of any guarantees about what SDNodes Arguments are translated into).
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
---|---|---|
5315 | Document that the output pairs are <Reg,Size>? | |
5334 | NB, this didn't compile for me (N needs to be dereferenced to get the SDNode inside). | |
5429–5434 | Reg.isVirtual()? As you're moving this code to use Register :) |
I got curious; the scenario I described happens in test/DebugInfo/MSP430/sdagsplit-1.ll, where one of the operands to a pair is a load.
The effect of reporting this as 0 seems to be avoiding extra copies of the same DBG_VALUE
Hmm, it looks like the extra DBG_VALUEs dropped in the msp430 test were legitimate locations for "b" -- if getUnderlyingArgRegs now fails but generates multiple {0,0} entries, the ArgRegsAndSizes.size() > 1 block on line 5487 generates no (well, zero sized) locations and returns true. Wheras before EmitFuncArgumentDbgValue returned false, and another code path found a location for "b".
IMO it's probably fine to just not handle (i.e. return false) cases where there's no ValueMap record and only partial information from getUnderlyingArgRegs -- it's almost certainly a rare circumstance, isn't necessary for your use cases, all we need to do is not risk generating _wrong_ locations.
Document that the output pairs are <Reg,Size>?