Page MenuHomePhabricator

[GlobalISel][InlineAsm] Fix buildCopy for inputs
ClosedPublic

Authored by Petar.Avramovic on Wed, Jul 8, 4:40 AM.

Details

Summary

Check that input size matches size of destination reg class.
Attempt to extend input size when needed.

Diff Detail

Event Timeline

Petar.Avramovic created this revision.Wed, Jul 8, 4:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 8, 4:40 AM

Hi @Petar.Avramovic,

we noticed that this extension is also missing for the non-tied register input operands. I added the opposite logic, i.e. truncating of output operands, but forgot about extending the inputs.
Would you mind adding that handling in this patch too, as it is basically the same?
If you feel like it should be in a separate commit, I can also provide a patch for it. Thanks!

Petar.Avramovic retitled this revision from [GlobalISel][InlineAsm] Fix buildCopy for matching input constraints to [GlobalISel][InlineAsm] Fix buildCopy for inputs.
Petar.Avramovic added a reviewer: kschwarz.
paquette added inline comments.Wed, Jul 8, 9:59 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
240

Would MachineIRBuilder::buildExtOrTrunc work here?

If not, maybe it would make sense to move this to MachineIRBuilder for the sake of consistency?

Petar.Avramovic marked an inline comment as done.Thu, Jul 9, 1:24 AM
Petar.Avramovic added inline comments.
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
240

The destination is vreg with reg class and source is generic vreg with LLT, it would not work for anyext since it requires both source and dest to be generic virtual registers. Here we anyext to new generic vreg with same size as Dst and then copy to Dst.
MachineIRBuilder specializes for generic vregs so should it be something like: "buildExtOrTruncToVRegWithRegClass" ? Should I also cover vectors? I don't know if there is a way to know LLT of vector type that would fit into reg class.

paquette added inline comments.Thu, Jul 9, 9:36 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
240

I see.

Considering that difference, I think this is fine. We can refactor later if it turns out to be a good idea.

As for vectors, I think that would be better in a follow-up.

254

Can we check MRI->getType(Src).isValid() before here? I think we always want to check this, right?

e.g.

auto SrcTy = MRI->getType(Src);
if (!SrcTy.isValid()) {
    LLVM_DEBUG(dbgs() << "Source type for copy is not valid\n");
    return false;
}

if (DstSize < SrcSize) {
  ...
}

// Attempt to anyext small scalar sources.
if (DstSize > SrcSize) {
  ...
}

...

Move isValid check on top.

This revision is now accepted and ready to land.Fri, Jul 10, 9:40 AM
This revision was automatically updated to reflect the committed changes.

I believe this has broken AArch64 (-global-isel not required due to defaults, but it's a GlobalISel bug so should force it nonetheless):

; RUN: llc -mtriple=aarch64 -global-isel < %s

define i64 @foo() nounwind {
  %1 = tail call i64 asm "", "={x2},0"(i64 0) nounwind
  ret i64 %1
}

fails with:

llc: /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/include/llvm/CodeGen/Register.h:79: static unsigned int llvm::Register::virtReg2Index(unsigned int): Assertion `isVirtualRegister(Reg) && "Not a virtual register"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /local/scratch/jrtc4/cheri/llvm-project-upstream/build/bin/llc -mtriple=aarch64 -O0
1.      Running pass 'Function Pass Manager' on module '<stdin>'.
2.      Running pass 'IRTranslator' on function '@foo'
 #0 0x00007fb5bc5e281b llvm::sys::PrintStackTrace(llvm::raw_ostream&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/Support/Unix/Signals.inc:564:0
 #1 0x00007fb5bc5e28ae PrintStackTraceSignalHandler(void*) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/Support/Unix/Signals.inc:625:0
 #2 0x00007fb5bc5e064b llvm::sys::RunSignalHandlers() /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/Support/Signals.cpp:68:0
 #3 0x00007fb5bc5e2198 SignalHandler(int) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/Support/Unix/Signals.inc:406:0
 #4 0x00007fb5bba4efd0 (/lib/x86_64-linux-gnu/libc.so.6+0x3efd0)
 #5 0x00007fb5bba4ef47 raise /build/glibc-2ORdQG/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #6 0x00007fb5bba508b1 abort /build/glibc-2ORdQG/glibc-2.27/stdlib/abort.c:81:0
 #7 0x00007fb5bba4042a __assert_fail_base /build/glibc-2ORdQG/glibc-2.27/assert/assert.c:89:0
 #8 0x00007fb5bba404a2 (/lib/x86_64-linux-gnu/libc.so.6+0x304a2)
 #9 0x00007fb5bb387afa llvm::Register::virtReg2Index(unsigned int) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/include/llvm/CodeGen/Register.h:79:0
#10 0x00007fb5bb388ad1 llvm::VirtReg2IndexFunctor::operator()(unsigned int) const /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/include/llvm/CodeGen/TargetRegisterInfo.h:1152:0
#11 0x00007fb5bb389c4f llvm::IndexedMap<std::pair<llvm::PointerUnion<llvm::TargetRegisterClass const*, llvm::RegisterBank const*>, llvm::MachineOperand*>, llvm::VirtReg2IndexFunctor>::operator[](unsigned int) const /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/include/llvm/ADT/IndexedMap.h:51:0
#12 0x00007fb5bb4003d5 llvm::MachineRegisterInfo::getRegClass(llvm::Register) const /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/include/llvm/CodeGen/MachineRegisterInfo.h:632:0
#13 0x00007fb5bb3fe0ec llvm::InlineAsmLowering::lowerInlineAsm(llvm::MachineIRBuilder&, llvm::CallBase const&, std::function<llvm::ArrayRef<llvm::Register> (llvm::Value const&)>) const /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp:459:0
#14 0x00007fb5bb3ca04a llvm::IRTranslator::translateInlineAsm(llvm::CallBase const&, llvm::MachineIRBuilder&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1701:0
#15 0x00007fb5bb3ca744 llvm::IRTranslator::translateCall(llvm::User const&, llvm::MachineIRBuilder&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:1759:0
#16 0x00007fb5bb3ce383 llvm::IRTranslator::translate(llvm::Instruction const&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/include/llvm/IR/Instruction.def:209:0
#17 0x00007fb5bb3d0f97 llvm::IRTranslator::runOnMachineFunction(llvm::MachineFunction&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2511:0
#18 0x00007fb5bfd0d33f llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/CodeGen/MachineFunctionPass.cpp:73:0
#19 0x00007fb5bcc07ee8 llvm::FPPassManager::runOnFunction(llvm::Function&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/IR/LegacyPassManager.cpp:1587:0
#20 0x00007fb5bcc08191 llvm::FPPassManager::runOnModule(llvm::Module&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/IR/LegacyPassManager.cpp:1633:0
#21 0x00007fb5bcc085b9 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/IR/LegacyPassManager.cpp:1702:0
#22 0x00007fb5bcc035d1 llvm::legacy::PassManagerImpl::run(llvm::Module&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/IR/LegacyPassManager.cpp:614:0
#23 0x00007fb5bcc08e45 llvm::legacy::PassManager::run(llvm::Module&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/lib/IR/LegacyPassManager.cpp:1830:0
#24 0x000055c7388da197 compileModule(char**, llvm::LLVMContext&) /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/tools/llc/llc.cpp:653:0
#25 0x000055c7388d8077 main /local/scratch/jrtc4/cheri/llvm-project-upstream/llvm/tools/llc/llc.cpp:360:0
#26 0x00007fb5bba31b97 __libc_start_main /build/glibc-2ORdQG/glibc-2.27/csu/../csu/libc-start.c:344:0
#27 0x000055c7388d727a _start (/local/scratch/jrtc4/cheri/llvm-project-upstream/build/bin/llc+0x2527a)

This is on commit 161882816540fc011554e4a820ab896278491b6a, so after D83763 too, but the test also fails on the 11 release branch, which does not have that revision, only this one.

Credit to @sorear for providing a minimal C test case that I then analysed further.

jrtc27 added a comment.EditedSat, Aug 1, 1:37 PM

Actually the code in the backtrace points at D82651, not this. I did however see buildAnyextOrCopy in a backtrace when playing around to get that minimal case, which is what brought me to this revision. Investigating further, it seems that this is because of D82651 not fully implementing this case, and future revisions not fixing that. Prior to that revision, inline asm would hit reportTranslationError and then fall back to SelectionDAG due to isGlobalISelAbortEnabled being true (provided -global-isel wasn't passed to override that, as is done in my test, but not the original C), but now, since it claims to be implemented but not correctly, it instead asserts deep down and is unable to fall back to SelectionDAG.

Thus, please do one of:

  1. Gate inline asm lowering behind an experimental flag so that the old behaviour of using SelectionDAG when inline asm is involved is restored
  1. Ensure full support for all the inline asm constructs
  1. Be very cautious in what inline asm is accepted, calling reportTranslationError eagerly if it looks like anything might go awry
  1. Revert the set of inline asm commits (seems a bit unnecessary given the other options, though maybe the right option for 11.x if not doing option 1?)

or something else appropriate. But this regression is affecting the 11.x release and hit by real-world code (in this case, seL4 on AArch64 compiled at -O0).