[PATCH] Generic support for sub/super registers in AsmPrinter::EmitDwarfRegOp()

[Eric Christopher]

Hi Adrian,

Few comments on Patch #1:

The function comments from Patch #2 would be nice in this patch :)

+  if (Reg < 0) {
+    if (!Indirect && !MLoc.isIndirect()) {
+      // Walk up the super-register chain until we find a valid number.

Looks like this is designed to fail if we have Reg < 0 and indirect? I
think? (Formatting is hard). At any rate an illuminating comment would
be great.

+  else emitDwarfRegOp(*this, Reg);

Silly formatting nit, new line please.

Patch #2:

(Love the '-' signs!)

+/// Emit an dwarf register operation.

"Emit a"

+  // Otherwise, attempt to find a covering set of sub-register numbers.
+  unsigned CurPos = 0;

Pretty sure I understand what's going on here, but if you could
explain the algorithm in the comments it'd help me from having to
stare at it in the future :)

+  if (CurPos > 0)
+    // We're done.
+    return;
+

This looks like an early exit for the code below, it might be
conceptually easier to read if you inverted the conditional and
explained it.

+  AP.OutStreamer.AddComment("nop (could not find a dwarf register number)");
+  AP.EmitInt8(dwarf::DW_OP_nop);

Thanks!

-eric


On Fri, Feb 14, 2014 at 7:54 PM, Adrian Prantl <aprantl at apple.com> wrote:
> Earlier this week (r201190) I added generic support for super-registers in AsmPrinter::EmitDwarfRegOp(). This patch does the opposite, by substituting multiple sub-registers for super-registers without valid DWARF numbers. The result is general enough that we can get rid of ARMAsmPrinter::EmitDwarfRegOp().
>
> While I think this patch is rather uncontroversial; it is removing code, so I'm posting this for review first.
>
> thanks,
> adrian
>
> PS: these two patches also from the basis for the updated revision of http://llvm-reviews.chandlerc.com/D2680 I just uploaded.
>
>