[FastISel] Don't select a CallInst as a BasicBlock in the SelectionDAG fallback if it has bundled ops

[dpaoliello]

This was discovered while looking at the codegen for x64 when Control Flow Guard is enabled.

When using SelectionDAG, LLVM would generate the following sequence for a CF guarded indirect call:

	leaq	target_func(%rip), %rax
	rex64 jmpq	*__guard_dispatch_icall_fptr(%rip) # TAILCALL

However, when Fast ISel was used the following is generated:

	leaq	target_func(%rip), %rax
	movq	__guard_dispatch_icall_fptr(%rip), %rcx
	rex64 jmpq	*%rcx                   # TAILCALL

This was happening despite Fast ISel aborting and falling back to SelectionDAG.

The root cause for this code gen is that SelectionDAGISel has a special case when Fast ISel aborts when lowering a CallInst where it tries to lower the instruction as its own basic block, which for such a CF Guard call means that it is lowering an indirect call to __guard_dispatch_icall_fptr without observing that the function was being loaded into a pointer in the preceding (and bundled) instruction.

The fix for this is to not use the special case when a CallInst has bundled instructions: it's better to allow the call and its bundled instructions to be lowered together by SelectionDAG instead.

[llvmbot]

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-selectiondag

Author: Daniel Paoliello (dpaoliello)

Changes

This was discovered while looking at the codegen for x64 when Control Flow Guard is enabled.

When using SelectionDAG, LLVM would generate the following sequence for a CF guarded indirect call:

	leaq	target_func(%rip), %rax
	rex64 jmpq	*__guard_dispatch_icall_fptr(%rip) # TAILCALL

However, when Fast ISel was used the following is generated:

	leaq	target_func(%rip), %rax
	movq	__guard_dispatch_icall_fptr(%rip), %rcx
	rex64 jmpq	*%rcx                   # TAILCALL

This was happening despite Fast ISel aborting and falling back to SelectionDAG.

The root cause for this code gen is that SelectionDAGISel has a special case when Fast ISel aborts when lowering a CallInst where it tries to lower the instruction as its own basic block, which for such a CF Guard call means that it is lowering an indirect call to __guard_dispatch_icall_fptr without observing that the function was being loaded into a pointer in the preceding (and bundled) instruction.

The fix for this is to not use the special case when a CallInst has bundled instructions: it's better to allow the call and its bundled instructions to be lowered together by SelectionDAG instead.

---

Full diff: https://github.com/llvm/llvm-project/pull/162895.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+8)
• (modified) llvm/test/CodeGen/X86/cfguard-checks.ll (+6-3)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index 175753f08d2b1..58539bd4cea3a 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -1825,6 +1825,14 @@ void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
 
           reportFastISelFailure(*MF, *ORE, R, EnableFastISelAbort > 2);
 
+          // If the call has operand bundles, then it's best if they are handled
+          // together with the call instead of selecting the call as its own
+          // block.
+          if (cast<CallInst>(Inst)->hasOperandBundles()) {
+            NumFastIselFailures += NumFastIselRemaining;
+            break;
+          }
+
           if (!Inst->getType()->isVoidTy() && !Inst->getType()->isTokenTy() &&
               !Inst->use_empty()) {
             Register &R = FuncInfo->ValueMap[Inst];
diff --git a/llvm/test/CodeGen/X86/cfguard-checks.ll b/llvm/test/CodeGen/X86/cfguard-checks.ll
index a727bbbfdcbe3..3a2de718e8a1b 100644
--- a/llvm/test/CodeGen/X86/cfguard-checks.ll
+++ b/llvm/test/CodeGen/X86/cfguard-checks.ll
@@ -1,7 +1,9 @@
 ; RUN: llc < %s -mtriple=i686-pc-windows-msvc | FileCheck %s -check-prefix=X86
-; RUN: llc < %s -mtriple=x86_64-pc-windows-msvc | FileCheck %s -check-prefixes=X64,X64_MSVC
+; RUN: llc < %s -mtriple=x86_64-pc-windows-msvc | FileCheck %s -check-prefixes=X64,X64_MSVC,X64_SELDAG
+; RUN: llc < %s --fast-isel -mtriple=x86_64-pc-windows-msvc | FileCheck %s -check-prefixes=X64,X64_MSVC,X64_FISEL
 ; RUN: llc < %s -mtriple=i686-w64-windows-gnu | FileCheck %s -check-prefixes=X86,X86_MINGW
-; RUN: llc < %s -mtriple=x86_64-w64-windows-gnu | FileCheck %s -check-prefixes=X64,X64_MINGW
+; RUN: llc < %s -mtriple=x86_64-w64-windows-gnu | FileCheck %s -check-prefixes=X64,X64_MINGW,X64_SELDAG
+; RUN: llc < %s --fast-isel -mtriple=x86_64-w64-windows-gnu | FileCheck %s -check-prefixes=X64,X64_MINGW,X64_FISEL
 ; Control Flow Guard is currently only available on Windows
 
 ; Test that Control Flow Guard checks are correctly added when required.
@@ -27,7 +29,8 @@ entry:
   ; X64-LABEL: func_guard_nocf
   ; X64:       leaq	target_func(%rip), %rax
   ; X64-NOT: __guard_dispatch_icall_fptr
-  ; X64:       callq	*%rax
+  ; X64_SELDAG: callq	*%rax
+  ; X64_FISEL: callq	*32(%rsp)
 }
 attributes #0 = { "guard_nocf" }
 

[arsenm]

I don't understand the flow here. These seems pretty far removed from where the problem is?