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

[Alexandre Yukio Yamashita via Phabricator via llvm-commits]

alexandreyy marked 2 inline comments as done.
alexandreyy added a comment.

In https://reviews.llvm.org/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.



================
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"));
----------------
nemanjai wrote:
> I'm not a fan of this name. We're not "ignoring" the percent sign here, we're just not printing it, right?
That's right. Fixed variable name to StripRegisterPrefix.


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


https://reviews.llvm.org/D39016