Skip to content
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.

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

release/18.x: [X86] Resolve FIXME: Enable PC relative calls on Windows #84185

Closed
wants to merge 1 commit into from

Conversation

AtariDreams
Copy link
Contributor

WinCOFFObjectWriter::RecordRelocation has been able to emit PC relative calls for a while now, but the workaround code has not been removed. We can safely enable it now for Windows.

…C-relative calls

This has been true for a while now, but this check has not been removed.
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 6, 2024

@llvm/pr-subscribers-backend-x86

Author: AtariDreams (AtariDreams)

Changes

WinCOFFObjectWriter::RecordRelocation has been able to emit PC relative calls for a while now, but the workaround code has not been removed. We can safely enable it now for Windows.


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

2 Files Affected:

  • (modified) llvm/lib/Target/X86/X86Subtarget.cpp (+1-4)
  • (modified) llvm/test/CodeGen/X86/call-imm.ll (+3-1)
diff --git a/llvm/lib/Target/X86/X86Subtarget.cpp b/llvm/lib/Target/X86/X86Subtarget.cpp
index 07f535685e8f97..74d9b1bd327ea1 100644
--- a/llvm/lib/Target/X86/X86Subtarget.cpp
+++ b/llvm/lib/Target/X86/X86Subtarget.cpp
@@ -238,10 +238,7 @@ X86Subtarget::classifyGlobalFunctionReference(const GlobalValue *GV,
 
 /// Return true if the subtarget allows calls to immediate address.
 bool X86Subtarget::isLegalToCallImmediateAddr() const {
-  // FIXME: I386 PE/COFF supports PC relative calls using IMAGE_REL_I386_REL32
-  // but WinCOFFObjectWriter::RecordRelocation cannot emit them.  Once it does,
-  // the following check for Win32 should be removed.
-  if (Is64Bit || isTargetWin32())
+  if (Is64Bit)
     return false;
   return isTargetELF() || TM.getRelocationModel() == Reloc::Static;
 }
diff --git a/llvm/test/CodeGen/X86/call-imm.ll b/llvm/test/CodeGen/X86/call-imm.ll
index b8f5a0cb9b4287..5628dc3faddc08 100644
--- a/llvm/test/CodeGen/X86/call-imm.ll
+++ b/llvm/test/CodeGen/X86/call-imm.ll
@@ -2,6 +2,7 @@
 ; RUN: llc < %s -mtriple=i386-apple-darwin -relocation-model=pic | FileCheck -check-prefix X86PIC %s
 ; RUN: llc < %s -mtriple=i386-pc-linux -relocation-model=dynamic-no-pic | FileCheck -check-prefix X86DYN %s
 ; RUN: llc < %s -mtriple=i386-pc-win32 -relocation-model=static | FileCheck -check-prefix X86WINSTA %s
+; RUN: llc < %s -mtriple=i386-pc-win32 -relocation-model=pic | FileCheck -check-prefix X86WINPIC %s
 
 ; Call to immediate is not safe on x86-64 unless we *know* that the
 ; call will be within 32-bits pcrel from the dest immediate.
@@ -21,5 +22,6 @@ entry:
 ; X86STA: {{call.*12345678}}
 ; X86PIC-NOT: {{call.*12345678}}
 ; X86DYN: {{call.*12345678}}
-; X86WINSTA: {{call.*[*]%eax}}
+; X86WINSTA: {{call.*12345678}}
+; X86WINPIC-NOT: {{call.*12345678}}
 ; X64: {{call.*[*]%rax}}

@RKSimon
Copy link
Collaborator

RKSimon commented Mar 6, 2024

Now that 18.1 has been released - we shouldn't be merging anything that isn't just a regression from 17.x

I've tried to find the release policy for this in case 18.2 is now allow further merges but I can't find anything?

@AtariDreams AtariDreams closed this Mar 8, 2024
@AtariDreams AtariDreams deleted the COFF branch March 8, 2024 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants