This is an archive of the discontinued LLVM Phabricator instance.

[llvm][SelectionDAGBuilder] codegen callbr.landingpad intrinsic
ClosedPublic

Authored by nickdesaulniers on Dec 15 2022, 1:01 PM.

Details

Summary

Given a CallBrInst, retain its first virtual register in SelectionDagBuilder's
FunctionLoweringInfo if there's corresponding landingpad. Walk the list
of COPY MachineInstr to find the original virtual and physical registers
defined by the INLINEASM_BR MachineInst.

Test cases from https://reviews.llvm.org/D139565.
Link: https://github.com/llvm/llvm-project/issues/59538

Part 3 from
https://discourse.llvm.org/t/rfc-syncing-asm-goto-with-outputs-with-gcc/65453/8

Follow up patches still need to wire up CallBrPrepare into the pass
pipelines.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 1:01 PM
nickdesaulniers requested review of this revision.Dec 15 2022, 1:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 1:01 PM
nickdesaulniers retitled this revision from [LLVM][SelectionDAGBuilder] codegen callbr.landingpad intrinsic to [llvm][SelectionDAGBuilder] codegen callbr.landingpad intrinsic.
  • update oneline to lowercase llvm for consistency
nickdesaulniers planned changes to this revision.Dec 15 2022, 2:07 PM

Some issues I hit in the next patch wiring this all up (i.e. removing TODOs from D139861):
MachineVerifier::visitMachineBasicBlockBefore will assert MBB has allocatable live-in, but isn't entry or landing-pad. for @test2 in llvm/test/CodeGen/X86/callbr-asm-outputs.ll. The fix is to additionally check !MBB->isInlineAsmBrIndirectTarget(). Now I hit an infinite loop in the compiler with that test file...

nickdesaulniers added a comment.EditedDec 15 2022, 2:43 PM

Some issues I hit in the next patch wiring this all up (i.e. removing TODOs from D139861):
MachineVerifier::visitMachineBasicBlockBefore will assert MBB has allocatable live-in, but isn't entry or landing-pad. for @test2 in llvm/test/CodeGen/X86/callbr-asm-outputs.ll. The fix is to additionally check !MBB->isInlineAsmBrIndirectTarget(). Now I hit an infinite loop in the compiler with that test file...

@test4 in that file. Will take a look. Looks like we're getting hung in CallBrPrepare though, way before SelectionDAGBuilder though. (https://reviews.llvm.org/D139872)

Fixed in https://reviews.llvm.org/D139872; going to re-upload this to remove "changes planned"

  • rekick/"ready for review"
  • s/-start-before=amdgpu-isel/x86-isel or aarch64-isel
  • rebase, format
nickdesaulniers planned changes to this revision.Jan 18 2023, 10:31 AM
nickdesaulniers added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11639

After a rebase this morning, I'm seeing:

[2218/5419] Building CXX object lib/CodeGen/Sel.../LLVMSelectionDAG.dir/SelectionDAGBuilder.cpp.o
/android0/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11641:40: warning: 'makeArrayRef<llvm::Type *>' is deprecated: Use deduction guide instead [-Wdeprecated-declarations]
                                     : makeArrayRef(CBRType);
                                       ^~~~~~~~~~~~
                                       ArrayRef
/android0/llvm-project/llvm/include/llvm/ADT/ArrayRef.h:509:3: note: 'makeArrayRef<llvm::Type *>' has been explicitly marked deprecated here
  LLVM_DEPRECATED("Use deduction guide instead", "ArrayRef")
  ^
  • fix deprecation warning and rebase
efriedma accepted this revision.Jan 25 2023, 9:38 AM

LGTM with a couple minor comments.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11621

The structure of this function seems more flexible than its actual purpose... I'd prefer to assert if the code has an unexpected structure. So maybe llvm_unreachable instead of return {};, and don't use recursion/a loop when you're expecting exactly one or two COPY instructions.

11671

You shouldn't need to explicitly mark kills at this point; they'll get inferred later.

This revision is now accepted and ready to land.Jan 25 2023, 9:38 AM
jyknight added inline comments.Jan 25 2023, 10:47 AM
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
44 ↗(On Diff #487925)

It's unfortunate to need to special case this. Perhaps the intrinsic should take the callbr's output value as an input -- instead of magically "poofing" its value into existence by looking at the terminating instruction of the predecessor block? Then this code could be deleted, I believe.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2126–2127

This change could also be reverted in that case I think, because it'd no longer appear spuriously unused.

llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
44 ↗(On Diff #487925)

I designed the intrinsic this way because of this comment by @efriedma : https://reviews.llvm.org/D139565#inline-1347398

Having the callbr as an operand seems redundant. We can already look up the corresponding callbr by just grabbing the terminator of the predecessor.

I might still have a prototype in a local branch that does have the explicit operand and uses an intrinsic rather than instruction, so it's probably not too bad to resurrect that code. I don't feel very strongly either way.

But I would like @efriedma to review @jyknight 's point above, since rewriting the tests is exhausting, and I would need to revise this patch and the two prior. @efriedma thoughts?

efriedma added inline comments.Jan 25 2023, 11:48 AM
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
44 ↗(On Diff #487925)

At that point, I was mostly concerned about consistency with other IR constructs like landingpad.
Given the current architecture (where it isn't really exposed to IR optimizations), I'm less concerned about that. So if adding the operand simplifies the code overall, that's fine.

I haven't thought through whether there might be other complications to changing the intrinsic that way

llvm/test/CodeGen/AArch64/callbr-asm-outputs-indirect-isel.ll
4

stale comment can now be removed

llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
44 ↗(On Diff #487925)

OK, here's the diff. It wasn't too bad, though rebasing the diffs for specific patches will be fun.
https://gist.github.com/nickdesaulniers/6df8e1a15f23636e2bf8abec96d425c7

@efriedma @jyknight PTAL and let me know your thoughts. I'll wait for your feedback before rebasing that down into the correct patches.

efriedma added inline comments.Jan 26 2023, 2:22 PM
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
44 ↗(On Diff #487925)

Looks fine? I mean, the simplification isn't really that big, but it doesn't really add any complexity elsewhere.

If it's going to be a pain to rebase, I'd be okay just committing it as a followup.

jyknight added inline comments.Jan 30 2023, 1:58 PM
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
44 ↗(On Diff #487925)

It feels like an improvement.

Also fine with me to commit separately, if you'd like to.

nickdesaulniers planned changes to this revision.Jan 30 2023, 3:41 PM
nickdesaulniers marked 5 inline comments as done.

Will update based on some of the outstanding feedback.

llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
44 ↗(On Diff #487925)

https://reviews.llvm.org/D142940 (marking thread as done, please reopen if there's more todo here)

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2126–2127

https://reviews.llvm.org/D142940 (marking thread as done, please reopen if there's more todo here)

nickdesaulniers marked 2 inline comments as done.Jan 30 2023, 3:51 PM
nickdesaulniers marked 2 inline comments as done.
  • remove 2 stale comments, refactor FollowCopyChain to be more rigid as per @efriedma
This revision is now accepted and ready to land.Jan 30 2023, 4:20 PM
void accepted this revision.Jan 31 2023, 4:47 PM
nickdesaulniers planned changes to this revision.Feb 6 2023, 10:13 AM

I'm getting a crash in visitCallBrLandingPad when building ARCH=i386 allyesconfig kernel builds.

creduced:

void emulator_cmpxchg_emulated() {
  int __ret = ({
    asm goto(""
             : "=@cc"
               "z"(*(long *)0)
             :
             :
             : efaultu64);
    __ret;
  });
efaultu64:
}
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11600–11601

Ok so given "=@cc" sort of constraints, we get:

INLINEASM_BR &"" [maystore] [attdialect], $0:[regdef:GR32], def %1:gr32, $1:[imm], %bb.2
%2:gr8 = SETCCr 4, implicit $eflags
%3:gr32 = MOVZX32rr8 killed %2:gr8
%0:gr64 = SUBREG_TO_REG 0, killed %3:gr32, %subreg.sub_32bit
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11600–11601

Looking at https://reviews.llvm.org/D57393,
it looks like I'm handling:

case TargetLowering::C_Register:
case TargetLowering::C_RegisterClass:

but not

case TargetLowering::C_Other:

I may need to rethink this approach.

nickdesaulniers added inline comments.Feb 6 2023, 1:05 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11600–11601

I have something hacked up locally using TLI.ComputeConstraintToUse that seems to work. That said, machine-cse is doing some wacky things with the results that looks like yet another bug.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11680

Another issue I'm hitting in real world kernel code is that this MVT is potentially not legal.

void strncpy_from_kernel_nofault_count() {
  asm goto("" : "=r"(*(char *)0) : : : Efault);
Efault:
}

when target powerpc64le-linux-gnu, the MVT here is i8, but the first legal MVT for the constraint's register class is i32.

efriedma added inline comments.Feb 13 2023, 2:22 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11680

Is that the only problem here? Consider:

void test(int *x) {
  asm goto("mov $0, %0" : "=r"(*x) : : : X);
X:;
}
efriedma added inline comments.Feb 13 2023, 2:25 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11680

Err, nevermind, got confused.

You might need to look at the logic we use to compute the types of inline asm calls.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
11680

Ok, I've basically rewritten this method in: https://reviews.llvm.org/D143961. PTAL.

(I might try again to squash D142940 and D143961 into their respective parent commits in this series)

This revision is now accepted and ready to land.Feb 16 2023, 2:56 PM