This is an archive of the discontinued LLVM Phabricator instance.

DAG: Handle dbg_value for arguments split into multiple subregs
ClosedPublic

Authored by arsenm on Jul 12 2019, 10:06 AM.

Details

Summary

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.

Diff Detail

Event Timeline

arsenm created this revision.Jul 12 2019, 10:06 AM
vsk added a subscriber: vsk.
vsk added inline comments.
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.

arsenm marked an inline comment as done.Jul 12 2019, 11:03 AM
arsenm added inline comments.
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

arsenm updated this revision to Diff 210157.Jul 16 2019, 1:11 PM

Use op_values

vsk added a comment.Jul 16 2019, 1:43 PM

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?

jmorse added a subscriber: jmorse.Jul 17 2019, 4:20 AM

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.

arsenm marked an inline comment as done.Jul 17 2019, 6:37 AM

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

arsenm updated this revision to Diff 210319.Jul 17 2019, 6:52 AM

Report 0 for unknown elements

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.

arsenm updated this revision to Diff 210716.Jul 18 2019, 5:35 PM

Don't report the other values

jmorse accepted this revision.Jul 19 2019, 2:04 AM

LGTM, thanks for the patch!

This revision is now accepted and ready to land.Jul 19 2019, 2:04 AM
arsenm closed this revision.Jul 19 2019, 6:36 AM

r366574