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.

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

Jump to bottom

[llvm] Mark win x64 SEH pseudo instruction as meta instructions #110889

Merged
merged 1 commit into from
Oct 2, 2024
Merged

[llvm] Mark win x64 SEH pseudo instruction as meta instructions #110889

merged 1 commit into from
Oct 2, 2024

Conversation

dpaoliello
Copy link
Contributor

When adding new SEH pseudo instructions in #110024 I noticed that some of the tests were changing their output since these new instructions were counting towards thresholds for branching versus folding decisions.

These instructions do not result in real machine instructions being emitted, so they should be marked as meta instructions.

@llvmbot
Copy link
Member

llvmbot commented Oct 2, 2024

@llvm/pr-subscribers-backend-x86

Author: Daniel Paoliello (dpaoliello)

Changes

When adding new SEH pseudo instructions in #110024 I noticed that some of the tests were changing their output since these new instructions were counting towards thresholds for branching versus folding decisions.

These instructions do not result in real machine instructions being emitted, so they should be marked as meta instructions.

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

2 Files Affected:

  • (modified) llvm/lib/Target/X86/X86InstrCompiler.td (+1-1)
  • (modified) llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll (+12-12)
diff --git a/llvm/lib/Target/X86/X86InstrCompiler.td b/llvm/lib/Target/X86/X86InstrCompiler.td
index 5a8177e2b3607b..0d9ce696bad467 100644
--- a/llvm/lib/Target/X86/X86InstrCompiler.td
+++ b/llvm/lib/Target/X86/X86InstrCompiler.td
@@ -235,7 +235,7 @@ let isBranch = 1, isTerminator = 1, isCodeGenOnly = 1 in {
 //===----------------------------------------------------------------------===//
 // Pseudo instructions used by unwind info.
 //
-let isPseudo = 1, SchedRW = [WriteSystem] in {
+let isPseudo = 1, isMeta = 1, SchedRW = [WriteSystem] in {
   def SEH_PushReg : I<0, Pseudo, (outs), (ins i32imm:$reg),
                             "#SEH_PushReg $reg", []>;
   def SEH_SaveReg : I<0, Pseudo, (outs), (ins i32imm:$reg, i32imm:$dst),
diff --git a/llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll b/llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll
index 6ed23b0d770cd4..3c06bd29b88739 100644
--- a/llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll
+++ b/llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll
@@ -17,7 +17,7 @@ define i32 @loopInfoSaveOutsideLoop(i32 %cond, i32 %N) #0 {
 ; ENABLE-NEXT:    .seh_pushreg %rbx
 ; ENABLE-NEXT:    .seh_endprologue
 ; ENABLE-NEXT:    testl %ecx, %ecx
-; ENABLE-NEXT:    je .LBB0_4
+; ENABLE-NEXT:    je .LBB0_5
 ; ENABLE-NEXT:  # %bb.1: # %for.preheader
 ; ENABLE-NEXT:    #APP
 ; ENABLE-NEXT:    nop
@@ -38,11 +38,11 @@ define i32 @loopInfoSaveOutsideLoop(i32 %cond, i32 %N) #0 {
 ; ENABLE-NEXT:    nop
 ; ENABLE-NEXT:    #NO_APP
 ; ENABLE-NEXT:    shll $3, %eax
-; ENABLE-NEXT:    jmp .LBB0_5
-; ENABLE-NEXT:  .LBB0_4: # %if.else
+; ENABLE-NEXT:    popq %rbx
+; ENABLE-NEXT:    retq
+; ENABLE-NEXT:  .LBB0_5: # %if.else
 ; ENABLE-NEXT:    movl %edx, %eax
 ; ENABLE-NEXT:    addl %edx, %eax
-; ENABLE-NEXT:  .LBB0_5: # %if.end
 ; ENABLE-NEXT:    popq %rbx
 ; ENABLE-NEXT:    retq
 ; ENABLE-NEXT:    .seh_endproc
@@ -53,7 +53,7 @@ define i32 @loopInfoSaveOutsideLoop(i32 %cond, i32 %N) #0 {
 ; DISABLE-NEXT:    .seh_pushreg %rbx
 ; DISABLE-NEXT:    .seh_endprologue
 ; DISABLE-NEXT:    testl %ecx, %ecx
-; DISABLE-NEXT:    je .LBB0_4
+; DISABLE-NEXT:    je .LBB0_5
 ; DISABLE-NEXT:  # %bb.1: # %for.preheader
 ; DISABLE-NEXT:    #APP
 ; DISABLE-NEXT:    nop
@@ -74,11 +74,11 @@ define i32 @loopInfoSaveOutsideLoop(i32 %cond, i32 %N) #0 {
 ; DISABLE-NEXT:    nop
 ; DISABLE-NEXT:    #NO_APP
 ; DISABLE-NEXT:    shll $3, %eax
-; DISABLE-NEXT:    jmp .LBB0_5
-; DISABLE-NEXT:  .LBB0_4: # %if.else
+; DISABLE-NEXT:    popq %rbx
+; DISABLE-NEXT:    retq
+; DISABLE-NEXT:  .LBB0_5: # %if.else
 ; DISABLE-NEXT:    movl %edx, %eax
 ; DISABLE-NEXT:    addl %edx, %eax
-; DISABLE-NEXT:  .LBB0_5: # %if.end
 ; DISABLE-NEXT:    popq %rbx
 ; DISABLE-NEXT:    retq
 ; DISABLE-NEXT:    .seh_endproc
@@ -157,7 +157,7 @@ define i32 @loopInfoSaveOutsideLoop2(i32 %cond, i32 %N) #0 {
 ; DISABLE-NEXT:    .seh_pushreg %rbx
 ; DISABLE-NEXT:    .seh_endprologue
 ; DISABLE-NEXT:    testl %ecx, %ecx
-; DISABLE-NEXT:    je .LBB1_4
+; DISABLE-NEXT:    je .LBB1_5
 ; DISABLE-NEXT:  # %bb.1: # %for.preheader
 ; DISABLE-NEXT:    #APP
 ; DISABLE-NEXT:    nop
@@ -178,11 +178,11 @@ define i32 @loopInfoSaveOutsideLoop2(i32 %cond, i32 %N) #0 {
 ; DISABLE-NEXT:    nop
 ; DISABLE-NEXT:    #NO_APP
 ; DISABLE-NEXT:    shll $3, %eax
-; DISABLE-NEXT:    jmp .LBB1_5
-; DISABLE-NEXT:  .LBB1_4: # %if.else
+; DISABLE-NEXT:    popq %rbx
+; DISABLE-NEXT:    retq
+; DISABLE-NEXT:  .LBB1_5: # %if.else
 ; DISABLE-NEXT:    addl %edx, %edx
 ; DISABLE-NEXT:    movl %edx, %eax
-; DISABLE-NEXT:  .LBB1_5: # %if.end
 ; DISABLE-NEXT:    popq %rbx
 ; DISABLE-NEXT:    retq
 ; DISABLE-NEXT:    .seh_endproc

@llvmbot
Copy link
Member

llvmbot commented Oct 2, 2024

@llvm/pr-subscribers-platform-windows

Author: Daniel Paoliello (dpaoliello)

Changes

When adding new SEH pseudo instructions in #110024 I noticed that some of the tests were changing their output since these new instructions were counting towards thresholds for branching versus folding decisions.

These instructions do not result in real machine instructions being emitted, so they should be marked as meta instructions.

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

2 Files Affected:

  • (modified) llvm/lib/Target/X86/X86InstrCompiler.td (+1-1)
  • (modified) llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll (+12-12)
diff --git a/llvm/lib/Target/X86/X86InstrCompiler.td b/llvm/lib/Target/X86/X86InstrCompiler.td
index 5a8177e2b3607b..0d9ce696bad467 100644
--- a/llvm/lib/Target/X86/X86InstrCompiler.td
+++ b/llvm/lib/Target/X86/X86InstrCompiler.td
@@ -235,7 +235,7 @@ let isBranch = 1, isTerminator = 1, isCodeGenOnly = 1 in {
 //===----------------------------------------------------------------------===//
 // Pseudo instructions used by unwind info.
 //
-let isPseudo = 1, SchedRW = [WriteSystem] in {
+let isPseudo = 1, isMeta = 1, SchedRW = [WriteSystem] in {
   def SEH_PushReg : I<0, Pseudo, (outs), (ins i32imm:$reg),
                             "#SEH_PushReg $reg", []>;
   def SEH_SaveReg : I<0, Pseudo, (outs), (ins i32imm:$reg, i32imm:$dst),
diff --git a/llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll b/llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll
index 6ed23b0d770cd4..3c06bd29b88739 100644
--- a/llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll
+++ b/llvm/test/CodeGen/X86/x86-win64-shrink-wrapping.ll
@@ -17,7 +17,7 @@ define i32 @loopInfoSaveOutsideLoop(i32 %cond, i32 %N) #0 {
 ; ENABLE-NEXT:    .seh_pushreg %rbx
 ; ENABLE-NEXT:    .seh_endprologue
 ; ENABLE-NEXT:    testl %ecx, %ecx
-; ENABLE-NEXT:    je .LBB0_4
+; ENABLE-NEXT:    je .LBB0_5
 ; ENABLE-NEXT:  # %bb.1: # %for.preheader
 ; ENABLE-NEXT:    #APP
 ; ENABLE-NEXT:    nop
@@ -38,11 +38,11 @@ define i32 @loopInfoSaveOutsideLoop(i32 %cond, i32 %N) #0 {
 ; ENABLE-NEXT:    nop
 ; ENABLE-NEXT:    #NO_APP
 ; ENABLE-NEXT:    shll $3, %eax
-; ENABLE-NEXT:    jmp .LBB0_5
-; ENABLE-NEXT:  .LBB0_4: # %if.else
+; ENABLE-NEXT:    popq %rbx
+; ENABLE-NEXT:    retq
+; ENABLE-NEXT:  .LBB0_5: # %if.else
 ; ENABLE-NEXT:    movl %edx, %eax
 ; ENABLE-NEXT:    addl %edx, %eax
-; ENABLE-NEXT:  .LBB0_5: # %if.end
 ; ENABLE-NEXT:    popq %rbx
 ; ENABLE-NEXT:    retq
 ; ENABLE-NEXT:    .seh_endproc
@@ -53,7 +53,7 @@ define i32 @loopInfoSaveOutsideLoop(i32 %cond, i32 %N) #0 {
 ; DISABLE-NEXT:    .seh_pushreg %rbx
 ; DISABLE-NEXT:    .seh_endprologue
 ; DISABLE-NEXT:    testl %ecx, %ecx
-; DISABLE-NEXT:    je .LBB0_4
+; DISABLE-NEXT:    je .LBB0_5
 ; DISABLE-NEXT:  # %bb.1: # %for.preheader
 ; DISABLE-NEXT:    #APP
 ; DISABLE-NEXT:    nop
@@ -74,11 +74,11 @@ define i32 @loopInfoSaveOutsideLoop(i32 %cond, i32 %N) #0 {
 ; DISABLE-NEXT:    nop
 ; DISABLE-NEXT:    #NO_APP
 ; DISABLE-NEXT:    shll $3, %eax
-; DISABLE-NEXT:    jmp .LBB0_5
-; DISABLE-NEXT:  .LBB0_4: # %if.else
+; DISABLE-NEXT:    popq %rbx
+; DISABLE-NEXT:    retq
+; DISABLE-NEXT:  .LBB0_5: # %if.else
 ; DISABLE-NEXT:    movl %edx, %eax
 ; DISABLE-NEXT:    addl %edx, %eax
-; DISABLE-NEXT:  .LBB0_5: # %if.end
 ; DISABLE-NEXT:    popq %rbx
 ; DISABLE-NEXT:    retq
 ; DISABLE-NEXT:    .seh_endproc
@@ -157,7 +157,7 @@ define i32 @loopInfoSaveOutsideLoop2(i32 %cond, i32 %N) #0 {
 ; DISABLE-NEXT:    .seh_pushreg %rbx
 ; DISABLE-NEXT:    .seh_endprologue
 ; DISABLE-NEXT:    testl %ecx, %ecx
-; DISABLE-NEXT:    je .LBB1_4
+; DISABLE-NEXT:    je .LBB1_5
 ; DISABLE-NEXT:  # %bb.1: # %for.preheader
 ; DISABLE-NEXT:    #APP
 ; DISABLE-NEXT:    nop
@@ -178,11 +178,11 @@ define i32 @loopInfoSaveOutsideLoop2(i32 %cond, i32 %N) #0 {
 ; DISABLE-NEXT:    nop
 ; DISABLE-NEXT:    #NO_APP
 ; DISABLE-NEXT:    shll $3, %eax
-; DISABLE-NEXT:    jmp .LBB1_5
-; DISABLE-NEXT:  .LBB1_4: # %if.else
+; DISABLE-NEXT:    popq %rbx
+; DISABLE-NEXT:    retq
+; DISABLE-NEXT:  .LBB1_5: # %if.else
 ; DISABLE-NEXT:    addl %edx, %edx
 ; DISABLE-NEXT:    movl %edx, %eax
-; DISABLE-NEXT:  .LBB1_5: # %if.end
 ; DISABLE-NEXT:    popq %rbx
 ; DISABLE-NEXT:    retq
 ; DISABLE-NEXT:    .seh_endproc

@dpaoliello dpaoliello mentioned this pull request Oct 2, 2024
Merged
efriedma-quic
efriedma-quic approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM

@dpaoliello dpaoliello merged commit db33d82 into llvm:main Oct 2, 2024
8 checks passed
@dpaoliello dpaoliello deleted the sehmeta branch October 2, 2024 23:52
@mstorsjo
Copy link
Member

mstorsjo commented Oct 4, 2024

Unfortunately, this caused errors, quite surprisingly, when compiling for i386. How that's possible, I'm not quite sure, but I've bisected and verified that the issue indeed is caused by this commit.

Repro:

using a = int;
class b {
public:
  void c();
};
struct B;
template <class> struct e { typedef B f; };
template <class g> struct h : e<g>::f {};
struct B {
  h<int> *operator->() { return static_cast<h<int> *>(this); }
  a i;
};
class j {
  using k = B;
  k d;

public:
  a l() { return d->i; }
};
class m {
  enum { aa };
  struct {
    char n[sizeof(void *)];
    j *ab;
    a m_fn4() const { return n[aa] ? ab->l() : 1; }
  } o;

public:
  bool p() const {
    a r = o.m_fn4();
    return r;
  }
};
bool q;
struct G {
  bool s() const;
  m ag;
};
bool G::s() const {
  q = ag.p();
  b().c();
}
$ clang -target i686-w64-mingw32 -w -c repro.cpp -O3 -g -gcodeview
error: directive must appear between .cv_fpo_proc and .cv_fpo_endprologue
error: directive must appear between .cv_fpo_proc and .cv_fpo_endprologue
2 errors generated.

The same issue also triggers for i686-windows-msvc targets (where the last -gcodeview is redundant).

I'm not quite sure why this happens, for i686, which shouldn't be using these SEH instructions at all.

But nevertheless, I'll go ahead and revert this commit to unbreak things, and then we can sort out the proper fix afterwards.

mstorsjo added a commit that referenced this pull request Oct 4, 2024
@mstorsjo
Revert "[llvm] Mark win x64 SEH pseudo instruction as meta instructio…
b837c9e 
…ns (#110889)"

This reverts commit db33d82.

This commit caused errors when compiling for i686 with codeview
debug info enabled, see
#110889 (comment)
for details.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
@dpaoliello @xgupta
[llvm] Mark win x64 SEH pseudo instruction as meta instructions (llvm…
5d27fcd 
…#110889)

When adding new SEH pseudo instructions in llvm#110024 I noticed that some
of the tests were changing their output since these new instructions
were counting towards thresholds for branching versus folding decisions.

These instructions do not result in real machine instructions being
emitted, so they should be marked as meta instructions.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
@mstorsjo @xgupta
Revert "[llvm] Mark win x64 SEH pseudo instruction as meta instructio…
45bfc0f 
…ns (llvm#110889)"

This reverts commit db33d82.

This commit caused errors when compiling for i686 with codeview
debug info enabled, see
llvm#110889 (comment)
for details.
dpaoliello added a commit to dpaoliello/llvm-project that referenced this pull request Oct 17, 2024
@dpaoliello
[llvm] Mark win x64 SEH pseudo instruction as meta instructions (llvm…
0add709 
…#110889)

When adding new SEH pseudo instructions in llvm#110024 I noticed that some
of the tests were changing their output since these new instructions
were counting towards thresholds for branching versus folding decisions.

These instructions do not result in real machine instructions being
emitted, so they should be marked as meta instructions.
dpaoliello added a commit to dpaoliello/llvm-project that referenced this pull request Oct 18, 2024
@dpaoliello
[llvm] Mark win x64 SEH pseudo instruction as meta instructions (llvm…
ca52a80 
…#110889)

When adding new SEH pseudo instructions in llvm#110024 I noticed that some
of the tests were changing their output since these new instructions
were counting towards thresholds for branching versus folding decisions.

These instructions do not result in real machine instructions being
emitted, so they should be marked as meta instructions.
@dpaoliello dpaoliello mentioned this pull request Oct 18, 2024
Merged
dpaoliello added a commit that referenced this pull request Oct 21, 2024
@dpaoliello
[llvm][x64] Mark win x64 SEH pseudo instruction as meta instructions …
6e1a7ac 
…(again) (#112962)

When adding new SEH pseudo instructions in #110024 I noticed that some
of the tests were changing their output since these new instructions
were counting towards thresholds for branching versus folding decisions.

These instructions do not result in real machine instructions being
emitted, so they should be marked as meta instructions.

This is a re-do of #110889 as we hit an issue where some of the SEH
pseudo instructions in the prolog were being duplicated, which resulted
errors being raised as the CodeView generator was seeing prolog
directives after an end-prolog directive:
<#110889 (comment)>.
The fix for this is to mark the prolog related SEH pseudo instructions
as being non-duplicatable.
EricWF pushed a commit to efcs/llvm-project that referenced this pull request Oct 22, 2024
@dpaoliello @EricWF
[llvm][x64] Mark win x64 SEH pseudo instruction as meta instructions …
51c16c9 
…(again) (llvm#112962)

When adding new SEH pseudo instructions in llvm#110024 I noticed that some
of the tests were changing their output since these new instructions
were counting towards thresholds for branching versus folding decisions.

These instructions do not result in real machine instructions being
emitted, so they should be marked as meta instructions.

This is a re-do of llvm#110889 as we hit an issue where some of the SEH
pseudo instructions in the prolog were being duplicated, which resulted
errors being raised as the CodeView generator was seeing prolog
directives after an end-prolog directive:
<llvm#110889 (comment)>.
The fix for this is to mark the prolog related SEH pseudo instructions
as being non-duplicatable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efriedma-quic efriedma-quic efriedma-quic approved these changes

Assignees
No one assigned
Labels
backend:X86 platform:windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dpaoliello @llvmbot @mstorsjo @efriedma-quic