Page MenuHomePhabricator

[DebugInfo] MCP: collect and update DBG_VALUEs encountered in local block
ClosedPublic

Authored by jmorse on Jan 3 2019, 4:35 AM.

Details

Summary

The purpose of this patch is to remove a user of collectDebugValues (via changeDebugValuesDefReg): it currently assumes that all DBG_VALUEs immediately follow the specified instruction, which isn't necessarily true. This is going to become very often untrue when PR38754 work is merged. It also cannot be fixed by collectDebugValues iterating over register users because MCP runs after regalloc.

Instead of calling changeDebugValuesDefReg on an instruction to change its debug users, in this patch we instead collect DBG_VALUEs of copies as we iterate over insns, and update the debug users of copies that are made dead. This isn't a non-functional change, because MCP will now update DBG_VALUEs that aren't immediately after a copy, but refer to the same register. I've hijacked the regression test for PR38773 to test for this new behaviour, an entirely new test seemed overkill.

While fiddling with this patch an obvious question came to me, of "Why don't we copy-propagate the debug users just like other instructions?". The reason why is that DBG_VALUEs are re-generated after regalloc, and don't get the 'renameable' flag assigned to their register operands, preventing them from being renamed here. (Plus, debug users have no register class). Addressing that seemed like quite a large modification for a small issue.

Diff Detail

Event Timeline

jmorse created this revision.Jan 3 2019, 4:35 AM
aprantl added inline comments.Jan 3 2019, 12:53 PM
lib/CodeGen/MachineCopyPropagation.cpp
252

Could we just pass a MachineInstr& instead, or can it be null for the !IsDebug case?

jmorse updated this revision to Diff 180217.Jan 4 2019, 3:47 AM
jmorse marked 2 inline comments as done.

Pass MI reference to ReadRegister, avoiding the need for more assertions.

lib/CodeGen/MachineCopyPropagation.cpp
252

We can indeed, uploading a new version now.

aprantl added inline comments.Jan 4 2019, 7:32 AM
lib/CodeGen/MachineCopyPropagation.cpp
213

instead of an opaque optional parameter, we could also define

enum { DebugUse = false, RegularUse = true };

possibly with better names and explicitly pass it every time. Up to you.

lib/CodeGen/MachineCopyPropagation.cpp
224

May be a more descriptive comment; something like:

/// Keeps track of debug users for the current basic block.

248–253

May be change the logic to:

if (IsDebug)
  CopyDbgUsers[Copy].push_back(&Reader);
else {
  LLVM_DEBUG(dbgs() << "MCP: Copy is used - not dead: "; Copy->dump());
  MaybeDeadCopies.remove(Copy);
}
627–631

As your intention is to replace the calls (direct and indirect) to collectDebugValues and changeDebugValuesDefReg, why not move the new code to a function that can be reused.

MaybeDead->someNewFunction(MaybeDead->getOperand(1).getReg(), CopyDbgUsers);
lib/CodeGen/MachineCopyPropagation.cpp
213

Same here.

jmorse updated this revision to Diff 181550.Jan 14 2019, 7:08 AM

Use enumeration and explicit function arguments to disambiguate debug and regular register readers

jmorse updated this revision to Diff 181553.Jan 14 2019, 7:09 AM
jmorse marked 2 inline comments as done.

Revise a comment (missed in previous rev, whoops)

jmorse marked 3 inline comments as done.Jan 14 2019, 7:35 AM
jmorse added inline comments.
lib/CodeGen/MachineCopyPropagation.cpp
627–631

I don't know that there's any useful abstraction occurring there -- once you've called MaybeDead->getOperand(1) (which is copy-instruction specific) the code ceases to be about any particular instruction. Possibly it could be a static MachineInstr function?

IMHO this is a direction to be avoided though: collectDebugValues gives the impression (via its name) that it's a function that works in the general case, but (IMHO again) all the users really have context-specific requirements. Repeating some code in these circumstances is useful to highlight the point that there's no easy (or general) solution.

jmorse marked an inline comment as done.Jan 14 2019, 8:56 AM
jmorse added inline comments.
lib/CodeGen/MachineCopyPropagation.cpp
627–631

Reading back that doesn't sound great, what I meant was: "Let's not try too hard to de-duplicate when IMHO all the users of collectDebugValues have very context specific requirements"

lib/CodeGen/MachineCopyPropagation.cpp
627–631

What I am suggesting is to encapsulate just the map processing into a function. But to preserve the logic that uses it.

jmorse updated this revision to Diff 181801.Jan 15 2019, 9:09 AM

Shift DBG_VALUE updating to a MachineRegisterInfo helper

jmorse marked an inline comment as done.Jan 15 2019, 9:10 AM
jmorse added inline comments.
lib/CodeGen/MachineCopyPropagation.cpp
627–631

Now seeing this is about the loop rather than the rest -- hows about this helper method in MRI? (The operation seems more register-info related than instr related).

Ping, @CarlosAlbertoEnciso how's the utility function?

Ancient-ping on this, with the aim of it helping the placeDbgValues patch in. To re-summarise: this collects DBG_VALUEs to rewrite while walking through MachineCopyPropagation, rather than relying on the all-dbg-values-follow-the-defining-inst assumption.

vsk accepted this revision.Jun 27 2019, 11:16 AM
vsk added a subscriber: bogner.

Lgtm, thanks for working on this. I haven't touched MCP myself, so could you wait for another +1? + @bogner in case he has time to take a look.

include/llvm/CodeGen/MachineRegisterInfo.h
819 ↗(On Diff #181801)

nit: consider ArrayRef? It's a bit more flexible and this is a widely-used class.

lib/CodeGen/MachineCopyPropagation.cpp
603

Wdyt of leaving a comment here about erasing CopyDbgUsers[MaybeDead] to save memory? There's probably a cpu-time tradeoff so it'd need profiling.

This revision is now accepted and ready to land.Jun 27 2019, 11:16 AM
bogner accepted this revision.Jul 8 2019, 3:36 PM

Seems reasonable to me

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 5:21 AM