This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Handle inline assembly clobber of link regsiter
ClosedPublic

Authored by stefanp on Apr 30 2021, 12:16 PM.

Details

Summary

This patch adds the handling of clobbers of the link register LR for inline
assembly.

This patch is to fix:
https://bugs.llvm.org/show_bug.cgi?id=50147

Diff Detail

Event Timeline

stefanp created this revision.Apr 30 2021, 12:16 PM
stefanp requested review of this revision.Apr 30 2021, 12:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 12:16 PM
stefanp added reviewers: lei, nemanjai, Restricted Project.Apr 30 2021, 12:18 PM
stefanp edited the summary of this revision. (Show Details)Apr 30 2021, 1:53 PM

I added this commit on top of a45fd436aef4d5712da99f8292f5d0b16794892c, built a master branch Linux kernel from ppc44x_defconfig, and booted in under QEMU 5.0.1 without any issues as before. Thanks for the quick fix!

You probably also want the following testcase:

define void @UseLR() {
  tail call void asm sideeffect "", "{lr}"(i32 1)
  ret void
}
stefanp updated this revision to Diff 343156.May 5 2021, 1:14 PM

Cleanup the source code according to clang format.

Minor changes for formatting.

You probably also want the following testcase:

define void @UseLR() {
  tail call void asm sideeffect "", "{lr}"(i32 1)
  ret void
}

I looked into doing this but PowerPC does not have enough support for special registers (like lr) for us to be able to do this test correctly. The backend does not recognize the copy of the value 1 to the LR register and it does not know how to correctly spill special registers. This is on our TODO list and we will address these issues in a separate patch. Also, at this point, the clang frontend does not recognize the lr register as a valid input register anyway. (This work is also on our TODO list.) The plan is to address both of these deficiencies in future patches.

Basically, if we try this example from C source we get:

$ cat useLR.c

void foo() {
  asm(""
      : 
      : "l"(1));
}

$ clang -O2 -S useLR.c
clang: /data/stefanp/Wyvern/main/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8657: void llvm::SelectionDAGBuilder::visitInlineAsm(const llvm::CallBase &): Assertion `(OpInfo.ConstraintType == TargetLowering::C_RegisterClass || OpInfo.ConstraintType == TargetLowering::C_Register) && "Unknown constraint type!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

We will add this test when we add more support for special regsiters on PowerPC.

Is there anything we can do to help get this merged? It would be nice to have this fixed in LLVM 12.0.1, which is going to have its first release candidate soon.

nemanjai accepted this revision.May 12 2021, 12:04 PM

LGTM other than the allocatable nit. Let's go with this fix for now to unblock the kernel build and we'll follow-up with a more complete fix for other reserved registers later.

llvm/lib/Target/PowerPC/PPCRegisterInfo.td
413

Should these have the let isAllocatable = 0 as well? Seems like they should.

This revision is now accepted and ready to land.May 12 2021, 12:04 PM
stefanp updated this revision to Diff 345100.May 13 2021, 5:42 AM

Added missing let isAllocatable = 0.

This revision was landed with ongoing or failed builds.May 13 2021, 5:43 AM
This revision was automatically updated to reflect the committed changes.
nickdesaulniers added inline comments.
llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll
77–95

@jyknight points out in review of D115688 that something looks fishy with this callbr unit test, and I think he's right (and I'm sorry I didn't catch this earlier in code review).

Let's say the asm string actually writes to lr rather than being simply a nop in this example, hence why it's in the clobber list. In that case, branching to the lr (returning) will not branch to the restored value of lr, but the one updated from the inline asm. I don't think that's correct.

Should the mflr/mtlr "surround" the user supplied asm? Otherwise this looks like a pointless spill+reload on the indirect branch.

llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll
77–95

Looks like prologepilog insertion is where things go wrong, perhaps?

That said, running just this IR function through:

$ llc -mtriple=powerpc64le-unknown-linux-unknown -verify-machineinstrs /android0/llvm-project/llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll  -ppc-asm-full-reg-names -stop-before=prologepilog -o callbr.mir
$ llc -run-pass=prologepilog callbr.mir -o -

Doesn't produce the same output as

$ llc -mtriple=powerpc64le-unknown-linux-unknown -verify-machineinstrs /android0/llvm-project/llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll  -ppc-asm-full-reg-names -stop-after=prologepilog -o -

does (when observing the results of prologepilog)...I must be holding MIR wrong. Though it looks like shrink-wrap may also be involved, IIUC?

diff --git a/llvm/lib/CodeGen/ShrinkWrap.cpp b/llvm/lib/CodeGen/ShrinkWrap.cpp
index f89069e9f728..67d6bc249ef9 100644
--- a/llvm/lib/CodeGen/ShrinkWrap.cpp
+++ b/llvm/lib/CodeGen/ShrinkWrap.cpp
@@ -504,6 +504,10 @@ bool ShrinkWrap::runOnMachineFunction(MachineFunction &MF) {
     }
 
     for (const MachineInstr &MI : MBB) {
+      if (MI.getOpcode() == TargetOpcode::INLINEASM_BR) {
+        LLVM_DEBUG(dbgs() << "inlineasm_br prevents shrink-wrapping\n");
+        return false;
+      }
       if (!useOrDefCSROrFI(MI, RS.get()))
         continue;
       // Save (resp. restore) point must dominate (resp. post dominate)

seems to fix this, but I suspect it's overly conservative? Let's discuss in D116424?