[PATCH] D39016: Add Percent Symbol In PPC Registers for Linux

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added subscribers: hfinkel, echristo.
nemanjai added a comment.

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.



================
Comment at: lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp:45
+static cl::opt<bool>
+IgnorePercentPrefix("ppc-ignore-percent-prefix", cl::Hidden, cl::init(false),
+             cl::desc("Prints register names without percent prefix"));
----------------
I'm not a fan of this name. We're not "ignoring" the percent sign here, we're just not printing it, right?


================
Comment at: lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp:459
+static const char *fixRegisterPrefix(const char *RegName, unsigned RegNum,
+                                     unsigned RegEncoding, bool isLinux) {
   if (FullRegNames) {
----------------
Naming convention: variables start with capitals.


================
Comment at: lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp:481
+      case 'v':
+      case 'c':
+        char *newRegName = new char[strlen(RegName) + 1];
----------------
With this, LLVM will show CR registers as `%crN`. I am not aware of any other tool that will produce this spelling.


https://reviews.llvm.org/D39016