This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Use MachineOperand::print in the MIRPrinter for MO_Register.
ClosedPublic

Authored by thegameg on Dec 5 2017, 6:12 AM.

Details

Summary

[CodeGen] Use MachineOperand::print in the MIRPrinter for MO_Register.

Work towards the unification of MIR and debug output by refactoring the interfaces.

For MachineOperand::print, keep a simple version that can be easily called from dump(), and a more complex one which will be called from both the MIRPrinter and MachineInstr::print.

Add extra checks inside MachineOperand for detached operands (operands with getParent() == nullptr).

Diff Detail

Event Timeline

thegameg created this revision.Dec 5 2017, 6:12 AM
MatzeB edited edge metadata.Dec 5 2017, 8:28 PM

Looks good, but I assume this is incomplete and cannot be pushed like this?

lib/CodeGen/MachineInstr.cpp
1245

I think in case of MachineInstr::print() (at least by default) we should print all register ties on machine operands. Not sure what your plans with this flag are anyway as it's not really used by MachineOperand::print yet?

lib/CodeGen/MachineOperand.cpp
264–268

Is this necessary for the printing changes?

360

This feels a bit strange to me in cases where the user already specified TRI/IntrinsicInfo/...

thegameg updated this revision to Diff 125695.Dec 6 2017, 4:17 AM
thegameg retitled this revision from [CodeGen] Unify printing API for MachineOperand between MIR and debug to [CodeGen] Use MachineOperand::print in the MIRPrinter for MO_Register..
thegameg edited the summary of this revision. (Show Details)
thegameg added reviewers: aprantl, qcolombet.
  • Move implementation from the MIRPrinter to MachineOperand.
  • Update tests.
thegameg added inline comments.Dec 6 2017, 4:25 AM
lib/CodeGen/LiveVariables.cpp
238

I also assumed <imp-kill> here meant implicit-use killed.

lib/CodeGen/MachineOperand.cpp
264–268

No, not necessary. I can commit this separately.

360

Indeed, it was one of the questions I was having: should we assume that if the user is using the "complex" printing method then we're not trying to get the target info for them (and remove the default arguments to make it more obvious) ? In other words, should we only call tryToGetTargetInfo on the first ::print method (and obviously document this behavior and do the same for MI, MBB, MF) ?

test/DebugInfo/MIR/X86/live-debug-vars-unused-arg-debugonly.mir
156

I don't know how much we want this debug-use to show up here. What do you think @aprantl?

MatzeB accepted this revision.Dec 6 2017, 11:27 AM

LGTM, thanks for thoroughly replacing the syntax in all the comments.

include/llvm/CodeGen/MachineBasicBlock.h
705

too much automatic replacements :) (and too fancy writing in the original comment)

lib/CodeGen/LiveVariables.cpp
238

yes it's an implicit use with a kill flag. Is there anything confusing here?

I know the syntax used in the comments isn't always consistent. We also have this strange thing in .mir where implicit-def is just another word for implicit def (but that is something to fix another day).

lib/CodeGen/MachineOperand.cpp
360

Yeah given that this is more of a convenience functionality rather than a core part of printing moving it to the other convenience print function makes sense to me.

lib/CodeGen/MachineSink.cpp
249

shouldn't this be implicit def dead

lib/CodeGen/RegAllocFast.cpp
275

killed

test/DebugInfo/MIR/X86/live-debug-vars-unused-arg-debugonly.mir
156

The MachineOperand isDebug flag is so obvious (it is exactly set on the operands of DBG_VALUE instructions and nowhere else) and is more of a shortcut to avoid a few MachineOperand::getParent()->isDebug() calls rather than a flag that contains real information that I'd vote to not print or parse it. (And just compute it when parsing .mir I believe this is already happening anyway). Of course we don't necessarily need to make this change in this patch.

This revision is now accepted and ready to land.Dec 6 2017, 11:27 AM
thegameg closed this revision.Dec 7 2017, 2:55 AM
thegameg marked 9 inline comments as done.

Committed as r320022 [CodeGen] Use MachineOperand::print in the MIRPrinter for MO_Register.

lib/CodeGen/LiveVariables.cpp
238

Right, I was wondering why not <imp-use,kill>.