PowerPC64: clang should pass CPU target to as when using -no-integrated-as

[Quuxplusone]

Bugzilla Link	PR31423
Status	NEW
Importance	P normal
Reported by	Anton Blanchard (anton@samba.org)
Reported on	2016-12-18 14:44:52 -0800
Last modified on	2017-06-01 22:12:52 -0700
Version	trunk
Hardware	PC Linux
CC	echristo@gmail.com, ehsanamiri@gmail.com, hfinkel@anl.gov, kit.barton@gmail.com, llvm-bugs@lists.llvm.org, nemanja.i.ibm@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

Kernel compiles have started failing as of Linux kernel commit:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f923efbcfdbaa4391874eeda676b08fcf9ad6e99

which changed an open coded two argument tlbiel to an instruction. gas has supported the two argument form of tlbiel for ages, the issue is we are calling gas without a CPU target:

/usr/bin/as -a64 -mppc64 -many -mlittle-endian -o foo.o

This is likely to cause other issues, such as incorrect branch prediction hints.

Anton

[Quuxplusone]

Proposed fix (Hal let me know what you think and if you think some test cases
should be added):

Index: lib/Driver/Tools.cpp
===================================================================
--- lib/Driver/Tools.cpp        (revision 289406)
+++ lib/Driver/Tools.cpp        (working copy)
@@ -109,6 +109,19 @@ static const char *getSparcAsmModeForCPU(StringRef
   }
 }

+static const char *getPPCAsmModeForCPU(StringRef Name) {
+  fprintf(stderr, "The CPU is: %s\n", Name.str().c_str());
+  return llvm::StringSwitch<const char *>(Name)
+        .Case("pwr7", "-mpower7")
+        .Case("power7", "-mpower7")
+        .Case("pwr8", "-mpower8")
+        .Case("power8", "-mpower8")
+        .Case("ppc64le", "-mpower8")
+        .Case("pwr9", "-mpower9")
+        .Case("power9", "-mpower9")
+        .Default("-many");
+}
+
 static void CheckPreprocessingOptions(const Driver &D, const ArgList &Args) {
   if (Arg *A = Args.getLastArg(options::OPT_C, options::OPT_CC)) {
     if (!Args.hasArg(options::OPT_E) && !Args.hasArg(options::OPT__SLASH_P) &&
@@ -9659,22 +9672,28 @@ void gnutools::Assembler::ConstructJob(Compilation
     else
       CmdArgs.push_back("--64");
     break;
-  case llvm::Triple::ppc:
+  case llvm::Triple::ppc: {
     CmdArgs.push_back("-a32");
     CmdArgs.push_back("-mppc");
-    CmdArgs.push_back("-many");
+    std::string CPU = getCPUName(Args, getToolChain().getTriple());
+    CmdArgs.push_back(getPPCAsmModeForCPU(CPU));
     break;
-  case llvm::Triple::ppc64:
+  }
+  case llvm::Triple::ppc64: {
     CmdArgs.push_back("-a64");
     CmdArgs.push_back("-mppc64");
-    CmdArgs.push_back("-many");
+    std::string CPU = getCPUName(Args, getToolChain().getTriple());
+    CmdArgs.push_back(getPPCAsmModeForCPU(CPU));
     break;
-  case llvm::Triple::ppc64le:
+  }
+  case llvm::Triple::ppc64le: {
     CmdArgs.push_back("-a64");
     CmdArgs.push_back("-mppc64");
-    CmdArgs.push_back("-many");
     CmdArgs.push_back("-mlittle-endian");
+    std::string CPU = getCPUName(Args, getToolChain().getTriple());
+    CmdArgs.push_back(getPPCAsmModeForCPU(CPU));
     break;
+  }
   case llvm::Triple::sparc:
   case llvm::Triple::sparcel: {
     CmdArgs.push_back("-32");

[Quuxplusone]

Thanks Nemanja. A simple test case based on what we hit in the kernel:

void foo(void)
{
    asm volatile("tlbiel 0,0");
}

[Quuxplusone]

Is this actually still an issue?
When I try this test case now, it assembles fine with -fno-integrated-as and
fails without it. This behaviour is unaffected by this patch.

[Quuxplusone]

Hi Nemanja, it still fails for me:

# clang -O2 -mcpu=power8 -c testcase.c
testcase.c:4:15: error: invalid operand for instruction
        asm volatile("tlbiel 0,0");
                     ^
<inline asm>:1:11: note: instantiated into assembly here
        tlbiel 0,0

                ^

The integrated assembler fails too:

# clang -O2 -no-integrated-as -mcpu=power8 -c testcase.c
testcase-445326.s: Assembler messages:
testcase-445326.s:11: Error: junk at end of line: `0'
clang-3.9: error: assembler command failed with exit code 1 (use -v to see
invocation)

gcc passes:

# gcc -O2 -mcpu=power8 -c /tmp/testcase.c

[Quuxplusone]

Out of curiosity does the integrated assembler not work here for you?

[Quuxplusone]

We were having issues with the dodgy things Linux does to create asm-offsets.h, but trying a build again I realise it has been fixed:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cf0c3e68aa81f992b0301f62e341b710d385bf68

I opened LLVM bug #33115 to track a fail with the integrated assembler with the same tlbiel X,Y instructions. With a workaround for that the kernel builds and boots.

[Quuxplusone]

Nemanja: That patch looks fine for the non-integrated assembler. If you want to clean it up and post with a testcase it should be a quick review.

[Quuxplusone]

The updated patch on Phabricator:
https://reviews.llvm.org/D33820