This is an archive of the discontinued LLVM Phabricator instance.

Differential D150047

[X86][CodeGen] Do not add `offset` for memory reference using symbol
ClosedPublic

Authored by alvinhochun on May 6 2023, 11:39 AM.

Details

Reviewers
epastor
hans
thakis
MaskRay
ayzhao
Commits
rG6b996282cee3: [X86][CodeGen] Do not add `offset` for memory reference using symbol
Summary

In the past, D71436 added writing the offset operator for some
legitimate cases. However, for memory references in Intel syntax, the
offset operator ([offset sym]) appears to be superfluous at best,
possibly wrong and contradictory at worst.

This patch bypasses writing the offset operator in
X86AsmPrinter::PrintIntelMemReference which affects exactly this
case. A similar code flow exists in X86IntelInstPrinter.cpp -
X86IntelInstPrinter::printMemReference.

The motivation for fixing this output is to allow us to reject the
confusing call [offset fn_ref] syntax in MC, as discussed in D149579.

Depends on D149579

Diff Detail

Event Timeline

alvinhochun created this revision.May 6 2023, 11:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2023, 11:39 AM
Herald added subscribers: pengfei, hiraditya. · View Herald Transcript
alvinhochun requested review of this revision.May 6 2023, 11:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2023, 11:39 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
alvinhochun added a child revision: D150048: [X86][MC] Reject `call`/`jmp [offset fn_ref]` in Intel syntax.May 6 2023, 11:41 AM
Harbormaster completed remote builds in B230439: Diff 520114.May 6 2023, 12:25 PM
MaskRay accepted this revision.May 6 2023, 12:58 PM

Thanks!

llvm/test/CodeGen/X86/ms-inline-asm-functions.ll
2

Add a comment `;; Check that the generated memory references do not contain the offset` operator. Use -no-integrated-as to disable AsmParser formatting.``

4

;; for non-CHECK non-RUN lines. This marker may help (future) auto tools to retain the comments.

94

Clean up unneeded attributes and metadata to reduce noise.

This revision is now accepted and ready to land.May 6 2023, 12:58 PM
MaskRay added inline comments.May 6 2023, 1:01 PM
llvm/test/CodeGen/X86/ms-inline-asm-functions.ll
2

Perhaps add -x86-asm-syntax=intel beside -no-integrated-as to output Intel syntax consistently.

alvinhochun added inline comments.May 7 2023, 2:59 AM
llvm/test/CodeGen/X86/ms-inline-asm-functions.ll
2

It doesn't seem to make a difference to the output of the inline assembly sections.

alvinhochun updated this revision to Diff 520159.May 7 2023, 3:05 AM

Apply suggestions.

Harbormaster completed remote builds in B230475: Diff 520159.May 7 2023, 3:36 AM
Closed by commit rG6b996282cee3: [X86][CodeGen] Do not add `offset` for memory reference using symbol (authored by alvinhochun). · Explain WhyMay 8 2023, 9:08 AM
This revision was automatically updated to reflect the committed changes.
alvinhochun added a commit: rG6b996282cee3: [X86][CodeGen] Do not add `offset` for memory reference using symbol.

Revision Contents

PathSize
llvm/
lib/
Target/
X86/
4 lines
test/
CodeGen/
X86/
105 lines
2 lines

Diff 520410

llvm/lib/Target/X86/X86AsmPrinter.cpp

Show First 20 Lines • Show All 504 Lines • ▼ Show 20 Lines if (IndexReg.getReg()) {
if (ScaleVal != 1) if (ScaleVal != 1)
O << ScaleVal << '*'; O << ScaleVal << '*';
PrintOperand(MI, OpNo + X86::AddrIndexReg, O); PrintOperand(MI, OpNo + X86::AddrIndexReg, O);
NeedPlus = true; NeedPlus = true;
} }
if (!DispSpec.isImm()) { if (!DispSpec.isImm()) {
if (NeedPlus) O << " + "; if (NeedPlus) O << " + ";
PrintOperand(MI, OpNo + X86::AddrDisp, O); // Do not add `offset` operator. Matches the behaviour of
// X86IntelInstPrinter::printMemReference.
PrintSymbolOperand(DispSpec, O);
} else { } else {
int64_t DispVal = DispSpec.getImm(); int64_t DispVal = DispSpec.getImm();
if (DispVal || (!IndexReg.getReg() && !HasBaseReg)) { if (DispVal || (!IndexReg.getReg() && !HasBaseReg)) {
if (NeedPlus) { if (NeedPlus) {
if (DispVal > 0) if (DispVal > 0)
O << " + "; O << " + ";
else { else {
O << " - "; O << " - ";
▲ Show 20 Lines • Show All 434 Lines • Show Last 20 Lines

llvm/test/CodeGen/X86/ms-inline-asm-functions.ll

  • This file was added.
;; Check that the generated memory references do not contain the `offset`
;; operator. Use `-no-integrated-as` to disable AsmParser formatting.
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Add a comment `;; Check that the generated memory references do not contain the offset` operator. Use -no-integrated-as to disable AsmParser formatting.``

MaskRay: Add a comment ``;; Check that the generated memory references do not contain the `offset`…
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Perhaps add -x86-asm-syntax=intel beside -no-integrated-as to output Intel syntax consistently.

MaskRay: Perhaps add `-x86-asm-syntax=intel` beside `-no-integrated-as` to output Intel syntax…
alvinhochunAuthorUnsubmitted
Done
ReplyInline Actions

It doesn't seem to make a difference to the output of the inline assembly sections.

alvinhochun: It doesn't seem to make a difference to the output of the inline assembly sections.
; RUN: llc -no-integrated-as -x86-asm-syntax=intel < %s | FileCheck %s
MaskRayUnsubmitted
Not Done
ReplyInline Actions

;; for non-CHECK non-RUN lines. This marker may help (future) auto tools to retain the comments.

MaskRay: `;;` for non-CHECK non-RUN lines. This marker may help (future) auto tools to retain the…
;; This file was compiled from clang/test/CodeGen/ms-inline-asm-functions.c,
;; using the following command line:
;;
;; bin/clang -cc1 -internal-isystem lib/clang/17/include -nostdsysteminc \
;; ../llvm-project/clang/test/CodeGen/ms-inline-asm-functions.c \
;; -triple i386-pc-windows-msvc -fms-extensions -S -o out.ll
source_filename = "/llvm-project/clang/test/CodeGen/ms-inline-asm-functions.c"
target datalayout = "e-m:x-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32-a:0:32-S32"
target triple = "i386-pc-windows-msvc"
@kptr = dso_local global ptr null, align 4
; Function Attrs: noinline nounwind optnone
define dso_local i32 @foo() #0 {
entry:
%r = alloca ptr, align 4
%call = call ptr @gptr()
store ptr %call, ptr %r, align 4
%0 = call i32 asm sideeffect inteldialect "call ${1:P}\0A\09call $2\0A\09call ${3:P}\0A\09call $4", "={eax},*m,*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(i32 (i32)) @k, ptr elementtype(ptr) %r, ptr elementtype(i32 (i32)) @kimport, ptr elementtype(ptr) @kptr) #3, !srcloc !4
; CHECK-LABEL: _foo:
; CHECK: #APP
; CHECK-NEXT: .intel_syntax
; CHECK-NEXT: call _k
; CHECK-NEXT: call [e{{([a-z]{2})}}]
; CHECK-NEXT: call [e{{([a-z]{2})}}]
; CHECK-NEXT: call [_kptr]
; CHECK-NEXT: .att_syntax
; CHECK-NEXT: #NO_APP
ret i32 %0
}
declare dso_local ptr @gptr()
declare dso_local i32 @k(i32 noundef)
declare dllimport i32 @kimport(i32 noundef)
; Function Attrs: noinline nounwind optnone
define dso_local i32 @bar() #0 {
entry:
%0 = call i32 asm sideeffect inteldialect "jmp ${1:P}\0A\09ja ${2:P}\0A\09JAE ${3:P}\0A\09LOOP ${4:P}\0A\09loope ${5:P}\0A\09loopne ${6:P}", "={eax},*m,*m,*m,*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(i32 (i32)) @k, ptr elementtype(i32 (i32)) @k, ptr elementtype(i32 (i32)) @k, ptr elementtype(i32 (i32)) @k, ptr elementtype(i32 (i32)) @k, ptr elementtype(i32 (i32)) @k) #3, !srcloc !5
; CHECK-LABEL: _bar:
; CHECK: #APP
; CHECK-NEXT: .intel_syntax
; CHECK-NEXT: jmp _k
; CHECK-NEXT: ja [_k]
; CHECK-NEXT: JAE [_k]
; CHECK-NEXT: LOOP [_k]
; CHECK-NEXT: loope [_k]
; CHECK-NEXT: loopne [_k]
; CHECK-NEXT: .att_syntax
; CHECK-NEXT: #NO_APP
ret i32 %0
}
; Function Attrs: noinline nounwind optnone
define dso_local i32 @baz() #0 {
entry:
%0 = call i32 asm sideeffect inteldialect "mov eax, $1\0A\09mov eax, $2", "=&{eax},*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(i32 (i32)) @k, ptr elementtype(ptr) @kptr) #3, !srcloc !6
; CHECK-LABEL: _baz:
; CHECK: #APP
; CHECK-NEXT: .intel_syntax
; CHECK-NEXT: mov eax, [_k]
; CHECK-NEXT: mov eax, [_kptr]
; CHECK-NEXT: .att_syntax
; CHECK-NEXT: #NO_APP
ret i32 %0
}
; Function Attrs: naked noinline nounwind optnone
define dso_local void @naked() #2 {
entry:
call void asm sideeffect inteldialect "pusha\0A\09call ${0:P}\0A\09popa\0A\09ret", "*m,~{eax},~{ebp},~{ebx},~{ecx},~{edi},~{edx},~{esi},~{esp},~{dirflag},~{fpsr},~{flags}"(ptr elementtype(i32 (i32)) @k) #3, !srcloc !7
; CHECK-LABEL: _naked:
; CHECK: #APP
; CHECK-NEXT: .intel_syntax
; CHECK-NEXT: pusha
; CHECK-NEXT: call _k
; CHECK-NEXT: popa
; CHECK-NEXT: ret
; CHECK-NEXT: .att_syntax
; CHECK-NEXT: #NO_APP
unreachable
}
attributes #0 = { noinline nounwind optnone }
attributes #2 = { naked noinline nounwind optnone }
attributes #3 = { nounwind }
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Clean up unneeded attributes and metadata to reduce noise.

MaskRay: Clean up unneeded attributes and metadata to reduce noise.
!llvm.module.flags = !{!0, !1, !2}
!llvm.ident = !{!3}
!0 = !{i32 1, !"NumRegisterParameters", i32 0}
!1 = !{i32 1, !"wchar_size", i32 2}
!2 = !{i32 1, !"MaxTLSAlign", i32 65536}
!3 = !{!"clang version 17.0.0"}
!4 = !{i64 528}
!5 = !{i64 892}
!6 = !{i64 1183}
!7 = !{i64 1451}

llvm/test/CodeGen/X86/ms-inline-asm.ll

Show First 20 Lines • Show All 85 Lines • ▼ Show 20 Linesentry:
%res = alloca ptr, align 4 %res = alloca ptr, align 4
call void asm sideeffect inteldialect "lea edi, dword ptr $0", "*m,~{edi},~{dirflag},~{fpsr},~{flags}"(ptr elementtype([2 x i32]) @results) nounwind call void asm sideeffect inteldialect "lea edi, dword ptr $0", "*m,~{edi},~{dirflag},~{fpsr},~{flags}"(ptr elementtype([2 x i32]) @results) nounwind
call void asm sideeffect inteldialect "mov dword ptr $0, edi", "=*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(ptr) %res) nounwind call void asm sideeffect inteldialect "mov dword ptr $0, edi", "=*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(ptr) %res) nounwind
%0 = load ptr, ptr %res, align 4 %0 = load ptr, ptr %res, align 4
ret ptr %0 ret ptr %0
; CHECK-LABEL: t30: ; CHECK-LABEL: t30:
; CHECK: {{## InlineAsm Start|#APP}} ; CHECK: {{## InlineAsm Start|#APP}}
; CHECK: .intel_syntax ; CHECK: .intel_syntax
; CHECK: lea edi, dword ptr [offset {{_?}}results] ; CHECK: lea edi, dword ptr [{{_?}}results]
; CHECK: .att_syntax ; CHECK: .att_syntax
; CHECK: {{## InlineAsm End|#NO_APP}} ; CHECK: {{## InlineAsm End|#NO_APP}}
; CHECK: {{## InlineAsm Start|#APP}} ; CHECK: {{## InlineAsm Start|#APP}}
; CHECK: .intel_syntax ; CHECK: .intel_syntax
; CHECK: mov dword ptr [esp], edi ; CHECK: mov dword ptr [esp], edi
; CHECK: .att_syntax ; CHECK: .att_syntax
; CHECK: {{## InlineAsm End|#NO_APP}} ; CHECK: {{## InlineAsm End|#NO_APP}}
; CHECK: movl (%esp), %eax ; CHECK: movl (%esp), %eax
▲ Show 20 Lines • Show All 63 Lines • Show Last 20 Lines