[llvm-ml] Fix RIP-relative addressing for ptr operands #107618
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-x86 Author: Andrew Ng (nga888) ChangesFixes #54773 Full diff: https://github.com/llvm/llvm-project/pull/107618.diff 2 Files Affected:
diff --git a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
index 03f49306c2b7b5..e88dc9cfbf4877 100644
--- a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -2707,7 +2707,7 @@ bool X86AsmParser::parseIntelOperand(OperandVector &Operands, StringRef Name) {
bool MaybeDirectBranchDest = true;
if (Parser.isParsingMasm()) {
- if (is64BitMode() && SM.getElementSize() > 0) {
+ if (is64BitMode() && (PtrInOperand || SM.getElementSize() > 0)) {
DefaultBaseReg = X86::RIP;
}
if (IsUnconditionalBranch) {
diff --git a/llvm/test/tools/llvm-ml/rip_relative_addressing.asm b/llvm/test/tools/llvm-ml/rip_relative_addressing.asm
index d237e84435b7d6..cdd984ee6a8522 100644
--- a/llvm/test/tools/llvm-ml/rip_relative_addressing.asm
+++ b/llvm/test/tools/llvm-ml/rip_relative_addressing.asm
@@ -53,4 +53,10 @@ mov eax, [t8]
; CHECK-LABEL: t8:
; CHECK: mov eax, dword ptr [t8]
-END
\ No newline at end of file
+t9:
+mov eax, dword ptr [bar]
+; CHECK-LABEL: t9:
+; CHECK-32: mov eax, dword ptr [bar]
+; CHECK-64: mov eax, dword ptr [rip + bar]
+
+END
|
nga888
force-pushed
the
fix-llvm-ml-rip-relative-addressing
branch
from
4aa238e to
52ea8e7
Compare
|
Thanks for working on this! Unfortunately, I'm not quite familiar enough with x86 assembly to comment reasonably on this... Hopefully someone else can ack it. |
phoebewang
reviewed
Sep 9, 2024
|
Have we confirmed that this fully fixes #54773 ? I'm prepared to approve this if so - it seems to match ml64's behavior - but I don't want us closing the other bug if this doesn't fix it! |
I have checked that using It was my own experiment of trying to replace MS |
|
Fantastic! LGTM. |
ericastor
approved these changes
Sep 9, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/5376 Here is the relevant piece of the build log for the reference |
|
I think it'd be great to have this fix widely deployed in 19.x as well, so we probably should file a backport request. But as it isn't entirely critical, and the bar for including things in 19.1.0 now is pretty high, we probably should file a backport request only after 19.1.0 is cut (early next week). |
mstorsjo
added a commit
that referenced
this pull request
Sep 10, 2024
…)" This reverts commit 7543d09. This change caused failed asserts when building the openmp assembly sources, reproducible with: $ llvm-ml -m64 -D_M_AMD64 -c -Fo out.obj openmp/runtime/src/z_Windows_NT-586_asm.asm llvm-ml: ../lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:624: void {anonymous}::X86MCCodeEmitter::emitMemModRMByte(const llvm::MCInst&, unsigned int, unsigned int, uint64_t, {anonymous}::PrefixKind, uint64_t, llvm::SmallVectorImpl<char>&, llvm::SmallVectorImpl<llvm::MCFixup>&, const llvm::MCSubtargetInfo&, bool) const: Assertion `IndexReg.getReg() == 0 && !ForceSIB && "Invalid rip-relative address"' failed. The assert can also be triggered with one lone instruction: lea rdx, QWORD PTR [rax*8+16]
|
Unfortunately, I had to revert this, as this triggered failed asserts when building the openmp runtime assembly sources. See 1581183 for details on how to build that, and for the single instruction that triggers the assert ( |
nga888
added a commit
to nga888/llvm-project
that referenced
this pull request
Sep 10, 2024
Relands llvm#107618 with fix for assertion triggered by OpenMP runtime MASM assembly source.
nga888
added a commit
to nga888/llvm-project
that referenced
this pull request
Sep 13, 2024
Relands llvm#107618 with fix for assertion triggered by OpenMP runtime MASM assembly source.
nga888
added a commit
that referenced
this pull request
Sep 13, 2024
Relands #107618 with fix for assertion triggered by OpenMP runtime MASM assembly source.
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Sep 18, 2024
…08061) Relands llvm#107618 with fix for assertion triggered by OpenMP runtime MASM assembly source. (cherry picked from commit 7574e1d)
tru
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Sep 24, 2024
…08061) Relands llvm#107618 with fix for assertion triggered by OpenMP runtime MASM assembly source. (cherry picked from commit 7574e1d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #54773