[X86] Avoid crashing in PIC mode on narrowing to i8 followed by extension to i32

[Ralender]

No description provided.

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: None (Ralender)

Changes

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp (+2-1)
• (added) llvm/test/CodeGen/X86/x86-access-to-global.ll (+27)

diff --git a/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp b/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
index 4b7a9127b3fc3..dfa6c06fe29f2 100644
--- a/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
@@ -631,7 +631,8 @@ void InstrEmitter::EmitSubregNode(SDNode *Node, VRBaseMapType &VRBaseMap,
 void
 InstrEmitter::EmitCopyToRegClassNode(SDNode *Node,
                                      VRBaseMapType &VRBaseMap) {
-  Register VReg = getVR(Node->getOperand(0), VRBaseMap);
+  RegisterSDNode *R = dyn_cast<RegisterSDNode>(Node->getOperand(0));
+  unsigned VReg = R ? R->getReg() : getVR(Node->getOperand(0), VRBaseMap);
 
   // Create the new VReg in the destination class and emit a copy.
   unsigned DstRCIdx = Node->getConstantOperandVal(1);
diff --git a/llvm/test/CodeGen/X86/x86-access-to-global.ll b/llvm/test/CodeGen/X86/x86-access-to-global.ll
new file mode 100644
index 0000000000000..9e09a035ac519
--- /dev/null
+++ b/llvm/test/CodeGen/X86/x86-access-to-global.ll
@@ -0,0 +1,27 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -relocation-model=pic < %s | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
+target triple = "i386-unknown-linux-gnu"
+
+@.str = external dso_local global i32
+
+define i1 @test() {
+; CHECK-LABEL: test:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    calll .L0$pb
+; CHECK-NEXT:    .cfi_adjust_cfa_offset 4
+; CHECK-NEXT:  .L0$pb:
+; CHECK-NEXT:    popl %eax
+; CHECK-NEXT:    .cfi_adjust_cfa_offset -4
+; CHECK-NEXT:  .Ltmp0:
+; CHECK-NEXT:    addl $_GLOBAL_OFFSET_TABLE_+(.Ltmp0-.L0$pb), %eax
+; CHECK-NEXT:    movl $.str@GOTOFF, %ecx
+; CHECK-NEXT:    addb %al, %cl
+; CHECK-NEXT:    sete %al
+; CHECK-NEXT:    retl
+  %i = ptrtoint ptr @.str to i8
+  %p = zext i8 %i to i32
+  %c = icmp eq i32 %p, 0
+  ret i1 %c
+}

[arsenm]

Suggested change

	unsigned VReg = R ? R->getReg() : getVR(Node->getOperand(0), VRBaseMap);
	Register VReg = R ? R->getReg() : getVR(Node->getOperand(0), VRBaseMap);

[arsenm]

This looks like a workaround that just happens to work in this situation. If I run your test I see the "Node emitted out of order - late"" assert, which usually indicates there's a cycle in the DAG

[topperc]

Looks like we have a COPY_TO_REGCLASS of X86ISD::GlobalBaseReg. X86ISD::GlobalBaseReg is selected as virtual register %0. But I guess it wasn't added to the VRBaseMap.

[topperc]

Looks like we have a COPY_TO_REGCLASS of X86ISD::GlobalBaseReg. X86ISD::GlobalBaseReg is selected as virtual register %0, but it's not in the VRBaseMap.

I don't think there's a cycle involved in this case.

[Ralender]

From what I remember, there is no cycle and the issue is what topperc described.
arsenm, do you still think its the wrong fix ?
I am not familiar with this part of the code.

[arsenm]

Still seems wrong. The value should already be in the map

[Ralender]

I looked around, it doesn't appear that X86ISD::GlobalBaseReg usually in the in the VRBaseMap.
it is usually handled by:

llvm-project/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

Line 408 in 372c808

} else if (RegisterSDNode *R = dyn_cast<RegisterSDNode>(Op)) {

or

llvm-project/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp

Line 1264 in 372c808

if (RegisterSDNode *R = dyn_cast<RegisterSDNode>(SrcVal))

So maybe it should be in the VRBaseMap but that is not the current behavior.

[arsenm]

This seems like a weird case where X86 invented a hack for a presumably constant register that avoids the chain use. Is there a reason this can't just use CopyFromReg for these in the first place? Alternatively we could have a TargetConstantRegister node type

[jayfoad]

Avoid crashing in PIC mode on narrowing to i8 followed by extention

Typo, should be "extension" :)

[Ralender]

Avoid crashing in PIC mode on narrowing to i8 followed by extention

Typo, should be "extension" :)

Fixed