Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@Ralender Ralender Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@Ralender Ralender Jun 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Create the new VReg in the destination class and emit a copy.
unsigned DstRCIdx = Node->getConstantOperandVal(1);
Expand Down
27 changes: 27 additions & 0 deletions llvm/test/CodeGen/X86/x86-access-to-global.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}