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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: include/llvm/CodeGen/MachineBasicBlock.h:705
 
-  /// Return whether (physical) register \p Reg has been <def>ined and not
+  /// Return whether (physical) register \p Reg has been ined and not
   /// <kill>ed as of just before \p Before.
----------------
too much automatic replacements :) (and too fancy writing in the original comment)


================
Comment at: lib/CodeGen/LiveVariables.cpp:238
     // AH =
-    // AL = ... <imp-def EAX>, <imp-kill AH>
     //    = AH
----------------
thegameg wrote:
> I also assumed `<imp-kill>` here meant `implicit-use killed`.
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).


================
Comment at: lib/CodeGen/MachineOperand.cpp:368
                            const TargetIntrinsicInfo *IntrinsicInfo) const {
+  tryToGetTargetInfo(*this, TRI, IntrinsicInfo);
+
----------------
thegameg wrote:
> MatzeB wrote:
> > This feels a bit strange to me in cases where the user already specified TRI/IntrinsicInfo/...
> 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) ?
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.


================
Comment at: lib/CodeGen/MachineSink.cpp:249
   //     ...
-  //     %reg16385<def> = DEC64_32r %reg16437, %eflags<imp-def,dead>
+  //     %reg16385 = DEC64_32r %reg16437, implicit dead %eflags
   //     ...
----------------
shouldn't this be `implicit def dead`


================
Comment at: lib/CodeGen/RegAllocFast.cpp:275
     // Otherwise we may end up in a situation like this:
-    // ... = (MO) physreg:sub1, physreg <implicit-use, kill>
+    // ... = (MO) physreg:sub1, implicit kill physreg
     // ... <== Here we would allow later pass to reuse physreg:sub1
----------------
`killed`


================
Comment at: test/DebugInfo/MIR/X86/live-debug-vars-unused-arg-debugonly.mir:156
 # CHECKDBG-LABEL: ********** EMITTING LIVE DEBUG VARIABLES **********
-# CHECKDBG-NEXT: !"argc,5"        [0B;0e):0 Loc0=%edi
+# CHECKDBG-NEXT: !"argc,5"        [0B;0e):0 Loc0=debug-use %edi
 # CHECKDBG-NEXT:         [0B;0e):0 %bb.0-160B
----------------
thegameg wrote:
> I don't know how much we want this `debug-use` to show up here. What do you think @aprantl?
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.


https://reviews.llvm.org/D40836