Add Percent Symbol In PPC Registers for Linux

[joerg]

Summary

This patch inserts a percent symbol before the register names on PowerPC
for Linux. This makes easier to recognize the register types in the
assembly code. That improves the syntax of the code disassembled by
LLDB.
Tests were changed to ignore the percent prefixes.

[gut]

Just to show how much the disassembly is changed. Consider the following instruction:

li %r3,0

currently, it's disassembled by llvm like:

0x20000780 <+0>:  li     3, 0

However this is what it looks like in gdb:

0x0000000020000780 <+0>:     li      r3,0

and in objdump -d:

780:   00 00 60 38     li      r3,0

The code currently disassembled by llvm is not wrong! But it is surely less readable when not placing register type hints.

Now how this patch makes it looks like:

li[0x780] <+0>:  li     %r3, 0

PS: Both %r3 and 3 are valid 1st parameter for li instruction on PowerPC.

[alexandreyy]

ping

[nemanjai]

A few general comments:

1. You should add reviewers to this review. At the minimum @hfinkel, @kbarton and @echristo. Maybe someone from FreeBSD if this affects them.
2. Why do we need to change the default and get the existing behaviour with an option? Why not the other way around?
3. Is this syntax accepted by all the assemblers (I think LLVM for PPC runs on FreeBSD as well, but I have no idea if FreeBSD returns true for isOSLinux()? We don't want to produce assembly that we can't assemble.

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

45

I'm not a fan of this name. We're not "ignoring" the percent sign here, we're just not printing it, right?

461

Naming convention: variables start with capitals.

490

With this, LLVM will show CR registers as %crN. I am not aware of any other tool that will produce this spelling.

[joerg]

The patch is not acceptable in the current form. This includes fixing the memory leaks.

On PowerPC, three different output forms have to be supported:
(1) register name without percent on Darwin
(2) plain register number on AIX
(3) register name with percent OR plain register number on anything else
I don't think there is a big reason for supporting the second option of (3) though. The least amount of change is to introduce a predicate that checks for !Darwin && !AIX and then prints the % in the assembler printer. It is better to just pass down the target directly than deciding in the higher layers which forms should be choosen.

[alexandreyy]

Update according to review comments

[alexandreyy]

In D39016#902039, @nemanjai wrote:

A few general comments:

1. You should add reviewers to this review. At the minimum @hfinkel, @kbarton and @echristo. Maybe someone from FreeBSD if this affects them.
2. Why do we need to change the default and get the existing behaviour with an option? Why not the other way around?
3. Is this syntax accepted by all the assemblers (I think LLVM for PPC runs on FreeBSD as well, but I have no idea if FreeBSD returns true for isOSLinux()? We don't want to produce assembly that we can't assemble.

1, 3: FreeBSD should return true for isOSFreeBSD. This patch only affects the Linux behavior.
2: The default should be changed to allow LLDB prints the register prefixes. The code is still accepted by the assemblers in Linux.

It is not possible to set this option by LLDB.

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

45

That's right. Fixed variable name to StripRegisterPrefix.

461

Fixed variable names.

[alexandreyy]

In D39016#902082, @joerg wrote:

The patch is not acceptable in the current form. This includes fixing the memory leaks.

On PowerPC, three different output forms have to be supported:
(1) register name without percent on Darwin
(2) plain register number on AIX
(3) register name with percent OR plain register number on anything else
I don't think there is a big reason for supporting the second option of (3) though. The least amount of change is to introduce a predicate that checks for !Darwin && !AIX and then prints the % in the assembler printer. It is better to just pass down the target directly than deciding in the higher layers which forms should be choosen.

Fixed memory leak problem.
Changed code to handle target in the assembler printer, passing the target directly in the class constructor.

[joerg]

This is even worse. You can't new[] and then free(). Please follow the suggestion on just embedding the prefix directly, if desirable.

[alexandreyy]

In D39016#903648, @joerg wrote:

This is even worse. You can't new[] and then free(). Please follow the suggestion on just embedding the prefix directly, if desirable.

Is the suggestion to keep the default without the prefixes?
Because it is not possible to change the cl option by LLDB.

[joerg]

In D39016#903937, @alexandreyy wrote:

Because it is not possible to change the cl option by LLDB.

There should be no need to. This is not a public interface anyway. Long term, all sources should be changed to consistently use the human-friendly names, but that's a separate issue.

[alexandreyy]

In D39016#903973, @joerg wrote:

In D39016#903937, @alexandreyy wrote:

Because it is not possible to change the cl option by LLDB.

There should be no need to. This is not a public interface anyway. Long term, all sources should be changed to consistently use the human-friendly names, but that's a separate issue.

I am confused. Does the LLVM will be changed to support human-friendly names in the future by default?
Is there another way to change it now?
The LLVM interface could be modified to accept an option for printing the prefixes.
But It would require a lot of changes.

[nemanjai]

If what we want is to name the registers with the percent sign prefix on all non-AIX, non-Darwin systems, why not just change the definitions in the .td files to include the prefix? That way you don't have to worry about this kind of hacky/leaky code to prepend the symbol to the register names. And on AIX/Darwin, we just strip the full prefix (percent symbol and letter). Considering that it is questionable whether LLVM will ever have full support on AIX and that Darwin support is likely to be pulled soon, this seems like the best choice.

Of course, we'd then need to decide how we want to print the CR registers (i.e. if we want to have the ability to print them the way binutils tools print them - 4*cr4+gt, etc.)

[alexandreyy]

Update according to review comments

[joerg]

Almost.

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

490

Should be a static bool showRegistersWithPercentPrefix(RegName(const char *RegName) member of PPCInstPrinter and just return true or false.

542

...and here if (showRegistersWithPercentPrefix(RegName) O << "%";. Rest of the RegName logic can remain.

[alexandreyy]

Changed printRegisterWithPercentPrefix to showRegistersWithPercentPrefix

[alexandreyy]

In D39016#904944, @nemanjai wrote:

If what we want is to name the registers with the percent sign prefix on all non-AIX, non-Darwin systems, why not just change the definitions in the .td files to include the prefix? That way you don't have to worry about this kind of hacky/leaky code to prepend the symbol to the register names. And on AIX/Darwin, we just strip the full prefix (percent symbol and letter). Considering that it is questionable whether LLVM will ever have full support on AIX and that Darwin support is likely to be pulled soon, this seems like the best choice.

Of course, we'd then need to decide how we want to print the CR registers (i.e. if we want to have the ability to print them the way binutils tools print them - 4*cr4+gt, etc.)

The prefixes are already being printed if they are not stripped in PPCInstPrinter.cpp.
And the default can be changed to include the percent without a option.
However, all the test files should be changed to consider different syntaxes for each OS.

[gut]

In D39016#903973, @joerg wrote:

Long term, all sources should be changed to consistently use the human-friendly names, but that's a separate issue.

What did you mean with "Long term"? I'm interested in having such a human-friendlier disassembler on LLDB and I noticed that this patch is not going to change that.

(I just want to know when this mode would reach LLDB.)

[alexandreyy]

Hi.
Are these changes ok?

[joerg]

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

534

I said make it a member function because the knowledge that Darwin doesn't get the percent should be part of it, not external...

[alexandreyy]

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

534

If the OS is Darwin, the register name can't be stripped too.
So, We need to check the OS in the external function printOperand.

Or I can not strip the register prefix in stripRegisterPrefix if the OS is Darwin.

What do you think it is better?

[joerg]

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

534

That's why I suggested to keep things separate and simple. First just decide if the '%' prefix is desired; afterwards, decide whether the register name should be stripped. Keeping the decisions separate makes the flow easier. They will become less entangled in the future too.

I haven't really made up my mind on whether supporting the AIX flavor long term is desirable, I let others shim in on that. But that's a separate follow-up.

[alexandreyy]

Update patch according to suggestions

[alexandreyy]

Are these changes ok?

[alexandreyy]

Hi, @joerg
Could you review/approve these changes?

[alexandreyy]

Hi,
Could someone review/approve this patch, please?
I don't have the permissions to commit it.
Thanks. ;)

[echristo]

I really appreciate you doing this work. This has been something I've wanted for some time :)

I've added a few things to this. Should also have a testcase as well.

Thanks!

-eric

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

457

Seems to be no reason to make it a member function?

473

Ditto.

489

Having this not be the direct inverse of the above is weird. Perhaps redo some of that logic?

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.h

23

"TT" is a standard Triple name.

[alexandreyy]

Updated patch according to review.

[alexandreyy]

In D39016#921483, @echristo wrote:

I really appreciate you doing this work. This has been something I've wanted for some time :)

I've added a few things to this. Should also have a testcase as well.

Thanks!

-eric

Thanks @echristo.
I modified the patch according to your comments.

[alexandreyy]

Fixed identation.

[alexandreyy]

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

457

Removed from class.

489

Better with that logic.
But it does not change the output printed.
The percent is optional, so it is inserted if the flag FullRegNamesWithPercent is enabled.

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.h

23

Changed to TT

[alexandreyy]

Hi, guys!
Are these changes Ok to commit?

[alexandreyy]

ping

[alexandreyy]

@joerg, @echristo, Could you check if these changes are Ok?
Thanks!

[alexandreyy]

In D39016#927612, @alexandreyy wrote:

@joerg, @echristo, Could you check if these changes are Ok?
Thanks!

ping

[joerg]

Split into verbose conditional register names into a separate function. We likely want to remove them going forward as they are a specific feature of the Darwin assembler and not wildly supported.

Create precise predicates for using % register names and stripped register names, decoupling the (incomplete) existing checks.

[alexandreyy]

Thanks, @joerg !
It looks better than before.

[alexandreyy]

Hi, @echristo @joerg

Could we commit this patch?

[joerg]

So the next steps if you have the time would IMO be:

(1) Convert the Darwinish verbose conditional codes into annotations:

bt 2, foo # condition: eq

or something like that. This removes some platform specific logic and can be applied unconditionally.

(2) Convert the existing test cases and drop the -ppc-reg-with-precent-prefix option. This is sadly mostly busy work unless you can script it.

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.h

 //===- PPCInstPrinter.h - Convert PPC MCInst to assembly syntax -*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This class prints an PPC MCInst to a .s file.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_POWERPC_INSTPRINTER_PPCINSTPRINTER_H
 #define LLVM_LIB_TARGET_POWERPC_INSTPRINTER_PPCINSTPRINTER_H
 
+#include "llvm/ADT/Triple.h"
 #include "llvm/MC/MCInstPrinter.h"
 
 namespace llvm {
 
 class PPCInstPrinter : public MCInstPrinter {
-  bool IsDarwin;
+  Triple TripleInfo;
 public:
   PPCInstPrinter(const MCAsmInfo &MAI, const MCInstrInfo &MII,
-                 const MCRegisterInfo &MRI, bool isDarwin)
-    : MCInstPrinter(MAI, MII, MRI), IsDarwin(isDarwin) {}
-  
-  bool isDarwinSyntax() const {
-    return IsDarwin;
-  }
-  
+                 const MCRegisterInfo &MRI, Triple T)
+    : MCInstPrinter(MAI, MII, MRI), TripleInfo(T) {}
   void printRegName(raw_ostream &OS, unsigned RegNo) const override;
   void printInst(const MCInst *MI, raw_ostream &O, StringRef Annot,
                  const MCSubtargetInfo &STI) override;
-  
   // Autogenerated by tblgen.
   void printInstruction(const MCInst *MI, raw_ostream &O);
   static const char *getRegisterName(unsigned RegNo);
-  
   bool printAliasInstr(const MCInst *MI, raw_ostream &OS);
   void printCustomAliasOperand(const MCInst *MI, unsigned OpIdx,
                                unsigned PrintMethodIdx,
                                raw_ostream &OS);
 
   void printOperand(const MCInst *MI, unsigned OpNo, raw_ostream &O);
   void printPredicateOperand(const MCInst *MI, unsigned OpNo,
                              raw_ostream &O, const char *Modifier = nullptr);

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp

 FullRegNames("ppc-asm-full-reg-names", cl::Hidden, cl::init(false),
              cl::desc("Use full register names when printing assembly"));
 
 // Useful for testing purposes. Prints vs{31-63} as v{0-31} respectively.
 static cl::opt<bool>
 ShowVSRNumsAsVR("ppc-vsr-nums-as-vr", cl::Hidden, cl::init(false),
              cl::desc("Prints full register names with vs{31-63} as v{0-31}"));
 
+// Prints full register names with percent symbol.
+static cl::opt<bool>
+FullRegNamesWithPercent("print-reg-with-percent-prefix",
+                         cl::Hidden, cl::init(false),
+                         cl::desc("Prints full register names with percent"));
+
 #define PRINT_ALIAS_INSTR
 #include "PPCGenAsmWriter.inc"
 
 void PPCInstPrinter::printRegName(raw_ostream &OS, unsigned RegNo) const {
   const char *RegName = getRegisterName(RegNo);
   if (RegName[0] == 'q' /* QPX */) {
     // The system toolchain on the BG/Q does not understand QPX register names
     // in .cfi_* directives, so print the name of the floating-point
void PPCInstPrinter::printTLSCall(const MCInst *MI, unsigned OpNo,
   O << refExp.getSymbol().getName();
   O << '(';
   printOperand(MI, OpNo+1, O);
   O << ')';
   if (refExp.getKind() != MCSymbolRefExpr::VK_None)
     O << '@' << MCSymbolRefExpr::getVariantKindName(refExp.getKind());
 }
 
-
 /// stripRegisterPrefix - This method strips the character prefix from a
 /// register name so that only the number is left.  Used by for linux asm.
 static const char *stripRegisterPrefix(const char *RegName, unsigned RegNum,
                                        unsigned RegEncoding) {
   if (FullRegNames) {
     if (RegNum >= PPC::CR0EQ && RegNum <= PPC::CR7UN) {
       const char *CRBits[] =
       { "lt", "gt", "eq", "un",
         "4*cr1+lt", "4*cr1+gt", "4*cr1+eq", "4*cr1+un",
         "4*cr2+lt", "4*cr2+gt", "4*cr2+eq", "4*cr2+un",
         "4*cr3+lt", "4*cr3+gt", "4*cr3+eq", "4*cr3+un",
         "4*cr4+lt", "4*cr4+gt", "4*cr4+eq", "4*cr4+un",
         "4*cr5+lt", "4*cr5+gt", "4*cr5+eq", "4*cr5+un",
         "4*cr6+lt", "4*cr6+gt", "4*cr6+eq", "4*cr6+un",
         "4*cr7+lt", "4*cr7+gt", "4*cr7+eq", "4*cr7+un"
       };
       return CRBits[RegEncoding];
     }
     return RegName;
   }
 
   switch (RegName[0]) {
   case 'r':
   case 'f':
   case 'q': // for QPX
   case 'v':
     if (RegName[1] == 's')
       return RegName + 2;
     return RegName + 1;
   case 'c': if (RegName[1] == 'r') return RegName + 2;
   }
 
   return RegName;
 }
 
+/// showRegistersWithPercentPrefix - Check if name has prefix to insert
+/// percent symbol.
+static bool showRegistersWithPercentPrefix(const char *RegName) {
+    if (FullRegNamesWithPercent) {
+        switch (RegName[0]) {
+        case 'r':
+        case 'f':
+        case 'q':
+        case 'v':
+        case 'c':
+            return true;
+        default:
+            return false;
+        }
+    }
+
+    return false;
+}
+
 void PPCInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
                                   raw_ostream &O) {
   const MCOperand &Op = MI->getOperand(OpNo);
   if (Op.isReg()) {
     unsigned Reg = Op.getReg();
 
     // There are VSX instructions that use VSX register numbering (vs0 - vs63)
     // as well as those that use VMX register numbering (v0 - v31 which
     // correspond to vs32 - vs63). If we have an instruction that uses VSX
     // numbering, we need to convert the VMX registers to VSX registers.
     // Namely, we print 32-63 when the instruction operates on one of the
     // VMX registers.
     // (Please synchronize with PPCAsmPrinter::printOperand)
     if ((MII.get(MI->getOpcode()).TSFlags & PPCII::UseVSXReg) &&
         !ShowVSRNumsAsVR) {
       if (PPCInstrInfo::isVRRegister(Reg))
         Reg = PPC::VSX32 + (Reg - PPC::V0);
       else if (PPCInstrInfo::isVFRegister(Reg))
         Reg = PPC::VSX32 + (Reg - PPC::VF0);
     }
 
     const char *RegName = getRegisterName(Reg);
-    // The linux and AIX assembler does not take register prefixes.
-    if (!isDarwinSyntax())
-      RegName = stripRegisterPrefix(RegName, Reg, MRI.getEncodingValue(Reg));
+
+    // AIX assembler does not take register prefixes.
+    // Darwin assembler take full register prefixes.
+    if (!TripleInfo.isOSDarwin()) {
+        if (showRegistersWithPercentPrefix(RegName)) {
+            // Print register with percent symbol and prefix.
+            O << "%";
+        } else {
+            // Print register without prefix.
+            RegName = stripRegisterPrefix(RegName, Reg,
+                                          MRI.getEncodingValue(Reg));
+        }
+    }
 
     O << RegName;
     return;
   }
 
   if (Op.isImm()) {
     O << Op.getImm();
     return;
   }
 
   assert(Op.isExpr() && "unknown operand kind in printOperand");
   Op.getExpr()->print(O, &MAI);
 }
 

lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp

createObjectTargetStreamer(MCStreamer &S, const MCSubtargetInfo &STI) {
   return new PPCTargetMachOStreamer(S);
 }
 
 static MCInstPrinter *createPPCMCInstPrinter(const Triple &T,
                                              unsigned SyntaxVariant,
                                              const MCAsmInfo &MAI,
                                              const MCInstrInfo &MII,
                                              const MCRegisterInfo &MRI) {
-  return new PPCInstPrinter(MAI, MII, MRI, T.isOSDarwin());
+  return new PPCInstPrinter(MAI, MII, MRI, T);
 }
 
 extern "C" void LLVMInitializePowerPCTargetMC() {
   for (Target *T :
        {&getThePPC32Target(), &getThePPC64Target(), &getThePPC64LETarget()}) {
     // Register the MC asm info.
     RegisterMCAsmInfoFn C(*T, createPPCMCAsmInfo);