[GlobalISel][InlineAsm] Add early return for memory inputs that need to be indirectified

[kschwarz]

Summary

D78319 introduced basic support for inline asm input operands in GlobalISel.
However, that patch did not handle the case where a memory input operand still needs to
be indirectified. Later code asserts that the memory operand is already indirect.

This patch adds an early return false to trigger the SelectionDAG fallback for now.

[arsenm]

Needs test

[kschwarz]

Add test extracted from pr68328.c

[thakis]

Looks like this breaks tests: http://45.33.8.238/linux/17750/step_12.txt

Please take a look, and revert if it takes a while to investigate.

[thakis]

Reverted in e0c155427408814ceff22d16352331006eb81b6b for now.

[kschwarz]

Re-opening for investigation

[kschwarz]

The new check also triggered for memory clobbers, which we already handle.
I've moved the check further down, where we already check that it is an input operand.

[kschwarz]

Re-landed in 5425cdc3adf9998aeaf587d93417bd2f4f1373c9

Sorry for not running the regression tests again locally before pushing, it was already quite late here..

[leonardchan]

Hi! I believe commits 5425cdc3adf9998aeaf587d93417bd2f4f1373c9 and 91063cf85a4038537731f582a27936187fb0759c lead to the crash we're seeing in https://bugs.llvm.org/show_bug.cgi?id=46028.

The bug has full details, but the tl;dr is that with those 2 commits, we're seeing bad assembly generated for the following code:

inline Atomic64 NoBarrier_CompareAndSwap(volatile Atomic64* ptr,
                                         Atomic64 old_value,
                                         Atomic64 new_value) {
  Atomic64 prev;
  int32_t temp;
  __asm__ __volatile__ (  // NOLINT
    "0:                                    \n\t"
    "ldxr %[prev], %[ptr]                  \n\t"
    "cmp %[prev], %[old_value]             \n\t"
    "bne 1f                                \n\t"
    "stxr %w[temp], %[new_value], %[ptr]   \n\t"
    "cbnz %w[temp], 0b                     \n\t"
    "1:                                    \n\t"
    : [prev]"=&r" (prev),
      [temp]"=&r" (temp),
      [ptr]"+Q" (*ptr)
    : [old_value]"IJr" (old_value),
      [new_value]"r" (new_value)
    : "cc", "memory"
  );  // NOLINT
  return prev;
}

produces

2c: 08 7d 5f c8                   ldxr    x8, [x8]
30: 1f 01 0a eb                   cmp     x8, x10
34: 61 00 00 54                   b.ne    0x40 <_ZN6google8protobuf8internal24NoBarrier_CompareAndSwapEPVlll+0x40>
38: 0b 7d 09 c8                   stxr    w9, x11, [x8]
3c: 89 ff ff 35                   cbnz    w9, 0x2c <_ZN6google8protobuf8internal24NoBarrier_CompareAndSwapEPVlll+0x2c>
40: e8 0b 00 f9                   str     x8, [sp, #16]
44: e9 0f 00 b9                   str     w9, [sp, #12]

when it should be making

2c: 0c 7d 5f c8                   ldxr    x12, [x8]
30: 9f 01 0a eb                   cmp     x12, x10
34: 61 00 00 54                   b.ne    0x40 <_ZN6google8protobuf8internal24NoBarrier_CompareAndSwapEPVlll+0x40>
38: 0b 7d 09 c8                   stxr    w9, x11, [x8]
3c: 89 ff ff 35                   cbnz    w9, 0x2c <_ZN6google8protobuf8internal24NoBarrier_CompareAndSwapEPVlll+0x2c>
40: ec 0b 00 f9                   str     x12, [sp, #16]
44: e9 0f 00 b9                   str     w9, [sp, #12]

The difference is that the fist case clobbers x8 in which can lead to reading from a bad address in ldxr x8, [x8]. We're only seeing this on our AArch64 debug builds, which I believe is the only configuration where global isel is used.

Could you take a look or revert those 2 patches? Thanks.

[kschwarz]

Hi @leonardchan, I'll take a look at the failure, thanks for the reproducer!

llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp

 //===-- lib/CodeGen/GlobalISel/InlineAsmLowering.cpp ----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
case InlineAsm::isInput: {
         unsigned OpFlags =
             InlineAsm::getFlagWord(InlineAsm::Kind_Imm, Ops.size());
         Inst.addImm(OpFlags);
         Inst.add(Ops);
         break;
       }
 
       if (OpInfo.ConstraintType == TargetLowering::C_Memory) {
+
+        if (!OpInfo.isIndirect) {
+          LLVM_DEBUG(dbgs()
+                     << "Cannot indirectify memory input operands yet\n");
+          return false;
+        }
+
         assert(OpInfo.isIndirect && "Operand must be indirect to be a mem!");
 
         unsigned ConstraintID =
             TLI->getInlineAsmMemConstraint(OpInfo.ConstraintCode);
         unsigned OpFlags = InlineAsm::getFlagWord(InlineAsm::Kind_Mem, 1);
         OpFlags = InlineAsm::getFlagWordForMem(OpFlags, ConstraintID);
         Inst.addImm(OpFlags);
         ArrayRef<Register> SourceRegs =

llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll

 ; FALLBACK-WITH-REPORT-ERR: warning: Instruction selection used fallback path for strict_align_feature
 ; FALLBACK-WITH-REPORT-OUT-LABEL: strict_align_feature
 define i64 @strict_align_feature(i64* %p) #0 {
   %x = load i64, i64* %p, align 1
   ret i64 %x
 }
 
 attributes #0 = { "target-features"="+strict-align" }
+
+; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to translate instruction: call
+; FALLBACK-WITH-REPORT-ERR: warning: Instruction selection used fallback path for direct_mem
+; FALLBACK-WITH-REPORT-OUT-LABEL: direct_mem
+define void @direct_mem(i32 %x, i32 %y) {
+entry:
+  tail call void asm sideeffect "", "imr,imr,~{memory}"(i32 %x, i32 %y)
+  ret void
+}