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

[PowerPC][NFC] Allow different orders of .extern in some test cases #89714

Closed
wants to merge 1 commit into from

Conversation

orcguru
Copy link
Contributor

@orcguru orcguru commented Apr 23, 2024

The order of .externs is irrelevant to the functionality, however reverse-iteration buildbot check that.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 23, 2024

@llvm/pr-subscribers-backend-powerpc

Author: Felix (Ting Wang) (orcguru)

Changes

The order of .externs is irrelevant to the functionality, however reverse-iteration buildbot check that.


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

3 Files Affected:

  • (modified) llvm/test/CodeGen/PowerPC/aix-tls-gd-double.ll (+8-8)
  • (modified) llvm/test/CodeGen/PowerPC/aix-tls-gd-int.ll (+8-8)
  • (modified) llvm/test/CodeGen/PowerPC/aix-tls-gd-longlong.ll (+8-8)
diff --git a/llvm/test/CodeGen/PowerPC/aix-tls-gd-double.ll b/llvm/test/CodeGen/PowerPC/aix-tls-gd-double.ll
index ae41b6b1301064..196c4a6e743f78 100644
--- a/llvm/test/CodeGen/PowerPC/aix-tls-gd-double.ll
+++ b/llvm/test/CodeGen/PowerPC/aix-tls-gd-double.ll
@@ -612,14 +612,14 @@ entry:
 
 ; External symbol reference checks for .__tls_get_addr/.__tls_get_mod
 
-; SMALL32: .extern .__tls_get_addr[PR]
-; SMALL32: .extern .__tls_get_mod[PR]
-; SMALL64: .extern .__tls_get_addr[PR]
-; SMALL64: .extern .__tls_get_mod[PR]
-; LARGE32: .extern .__tls_get_addr[PR]
-; LARGE32: .extern .__tls_get_mod[PR]
-; LARGE64: .extern .__tls_get_addr[PR]
-; LARGE64: .extern .__tls_get_mod[PR]
+; SMALL32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
 
 ; TOC entry checks
 
diff --git a/llvm/test/CodeGen/PowerPC/aix-tls-gd-int.ll b/llvm/test/CodeGen/PowerPC/aix-tls-gd-int.ll
index bbb8e04b67b95e..6814dbe9e457a8 100644
--- a/llvm/test/CodeGen/PowerPC/aix-tls-gd-int.ll
+++ b/llvm/test/CodeGen/PowerPC/aix-tls-gd-int.ll
@@ -627,14 +627,14 @@ entry:
 
 ; External symbol reference checks for .__tls_get_addr/.__tls_get_mod
 
-; SMALL32: .extern .__tls_get_addr[PR]
-; SMALL32: .extern .__tls_get_mod[PR]
-; SMALL64: .extern .__tls_get_addr[PR]
-; SMALL64: .extern .__tls_get_mod[PR]
-; LARGE32: .extern .__tls_get_addr[PR]
-; LARGE32: .extern .__tls_get_mod[PR]
-; LARGE64: .extern .__tls_get_addr[PR]
-; LARGE64: .extern .__tls_get_mod[PR]
+; SMALL32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
 
 ; TOC entry checks
 
diff --git a/llvm/test/CodeGen/PowerPC/aix-tls-gd-longlong.ll b/llvm/test/CodeGen/PowerPC/aix-tls-gd-longlong.ll
index ff087a2144488c..54d954125b4cda 100644
--- a/llvm/test/CodeGen/PowerPC/aix-tls-gd-longlong.ll
+++ b/llvm/test/CodeGen/PowerPC/aix-tls-gd-longlong.ll
@@ -667,14 +667,14 @@ entry:
 
 ; External symbol reference checks for .__tls_get_addr/.__tls_get_mod
 
-; SMALL32: .extern .__tls_get_addr[PR]
-; SMALL32: .extern .__tls_get_mod[PR]
-; SMALL64: .extern .__tls_get_addr[PR]
-; SMALL64: .extern .__tls_get_mod[PR]
-; LARGE32: .extern .__tls_get_addr[PR]
-; LARGE32: .extern .__tls_get_mod[PR]
-; LARGE64: .extern .__tls_get_addr[PR]
-; LARGE64: .extern .__tls_get_mod[PR]
+; SMALL32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; SMALL64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
+; LARGE64: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
 
 ; TOC entry checks
 

; LARGE32: .extern .__tls_get_mod[PR]
; LARGE64: .extern .__tls_get_addr[PR]
; LARGE64: .extern .__tls_get_mod[PR]
; SMALL32: .extern .{{__tls_get_addr|__tls_get_mod}}[PR]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please explain why the order changes with/without your change about supporting local dynamic TLS mode? Thanks very much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the LD patch there is only .__tls_get_addr[PR]
After the LD patch, there is .__tls_get_addr[PR] and .__tls_get_mod[PR]

The LD patch turned some variables from GD to LD, so created new extern

Copy link
Collaborator

@chenzheng1030 chenzheng1030 Apr 24, 2024

Choose a reason for hiding this comment

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

OK. One more question is do we know why TLS external function calls are impacted by the iteration order, while for below cases:

int foo();
int bar();

int test() {
  return foo() + bar();
}

The external foo() and bar() are the same with both iteration order.

Could you please help to figure that out? And yes, like @efriedma-quic said, we may need to fix this in the compiler, not the test case.

Thanks a lot.

@efriedma-quic
Copy link
Collaborator

We actually care that the output of the compiler is deterministic, not just that the tests pass. This is important for reproducible builds.

So I don't think changing the CHECK lines is right here. Probably the compiler should sort the symbols before it outputs them, or something like that.

@orcguru
Copy link
Contributor Author

orcguru commented Apr 24, 2024

Sure, let me dig out the root cause, and hope somebody can fix that.

ExtSymSDNodeSymbols
It's definition was: SmallPtrSet<MCSymbol *, 8> ExtSymSDNodeSymbols;

New Note

Basically that's why we saw the reorder.

@orcguru orcguru closed this Apr 24, 2024
@chenzheng1030
Copy link
Collaborator

To let the community be aware, @bzEq will take over this issue. Thanks very much Kai. And thank you for addressing the root cause @orcguru

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

4 participants