Skip to content

[SPIRV][test] Make debug info test robust to unix/windows differences #105965

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

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

bogner
Copy link
Contributor

@bogner bogner commented Aug 25, 2024

I don't know if this is the right thing to do or if the emitter should be normalizing path separators, but this gets this test to pass on windows where sys::path::append will use "" instead of "/".

This is another follow up to #97558, as #105889 managed to fix the build, but not all of the tests.

I don't know if this is the right thing to do or if the emitter should
be normalizing path separators, but this gets this test to pass on
windows where `sys::path::append` will use "\" instead of "/".
@llvmbot
Copy link
Member

llvmbot commented Aug 25, 2024

@llvm/pr-subscribers-backend-spir-v

Author: Justin Bogner (bogner)

Changes

I don't know if this is the right thing to do or if the emitter should be normalizing path separators, but this gets this test to pass on windows where sys::path::append will use "" instead of "/".

This is another follow up to #97558, as #105889 managed to fix the build, but not all of the tests.


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

1 Files Affected:

  • (modified) llvm/test/CodeGen/SPIRV/debug-info/basic-global-di.ll (+3-3)
diff --git a/llvm/test/CodeGen/SPIRV/debug-info/basic-global-di.ll b/llvm/test/CodeGen/SPIRV/debug-info/basic-global-di.ll
index 336b7db324c3d4..139a8e11182bcc 100644
--- a/llvm/test/CodeGen/SPIRV/debug-info/basic-global-di.ll
+++ b/llvm/test/CodeGen/SPIRV/debug-info/basic-global-di.ll
@@ -8,12 +8,12 @@
 ; CHECK-MIR-DAG: [[dwarf_version:%[0-9]+\:iid\(s32\)]] = OpConstantI [[type_i64]], 5
 ; CHECK-MIR-DAG: [[source_language:%[0-9]+\:iid\(s32\)]] = OpConstantI [[type_i64]], 3
 ; CHECK-MIR-DAG: [[debug_info_version:%[0-9]+\:iid\(s32\)]] = OpConstantI [[type_i64]], 21
-; CHECK-MIR-DAG: [[filename_str:%[0-9]+\:id\(s32\)]] = OpString 1094795567, 1094795585, 792805697, 1111638594, 1111638594, 1128481583, 1128481603, 1697596227, 1886216568, 1663985004, 0
+; CHECK-MIR-DAG: [[filename_str:%[0-9]+\:id\(s32\)]] = OpString 1094795567, 1094795585, 792805697, 1111638594, 1111638594, 1128481583, 1128481603, {{1697596227|1700545347}}, 1886216568, 1663985004, 0
 ; CHECK-MIR-DAG: [[debug_source:%[0-9]+\:id\(s32\)]] = OpExtInst [[type_void]], 3, 35, [[filename_str]]
 ; CHECK-MIR-DAG: [[debug_compilation_unit:%[0-9]+\:id\(s32\)]] = OpExtInst [[type_void]], 3, 1, [[source_language]], [[dwarf_version]], [[debug_source]], [[debug_info_version]]
 
 ; CHECK-SPIRV: [[ext_inst_non_semantic:%[0-9]+]] = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
-; CHECK-SPIRV: [[filename_str:%[0-9]+]] = OpString "/AAAAAAAAAA/BBBBBBBB/CCCCCCCCC/example.c"
+; CHECK-SPIRV: [[filename_str:%[0-9]+]] = OpString "/AAAAAAAAAA/BBBBBBBB/CCCCCCCCC{{[/\\]}}example.c"
 ; CHECK-SPIRV-DAG: [[type_void:%[0-9]+]] = OpTypeVoid
 ; CHECK-SPIRV-DAG: [[type_i32:%[0-9]+]] = OpTypeInt 32 0
 ; CHECK-SPIRV-DAG: [[dwarf_version:%[0-9]+]] = OpConstant [[type_i32]] 5
@@ -23,7 +23,7 @@
 ; CHECK-SPIRV: [[debug_compiation_unit:%[0-9]+]] = OpExtInst [[type_void]] [[ext_inst_non_semantic]] DebugCompilationUnit [[source_language]] [[dwarf_version]] [[debug_source]] [[debug_info_version]]
 
 ; CHECK-OPTION-NOT: OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
-; CHECK-OPTION-NOT: OpString "/AAAAAAAAAA/BBBBBBBB/CCCCCCCCC/example.c"
+; CHECK-OPTION-NOT: OpString "/AAAAAAAAAA/BBBBBBBB/CCCCCCCCC{{[/\\]}}example.c"
 
 define spir_func void @foo() {
 entry:

Copy link
Contributor

@coopp coopp left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Assuming I've understood the new number appropriately, this looks good to me

@@ -8,12 +8,12 @@
; CHECK-MIR-DAG: [[dwarf_version:%[0-9]+\:iid\(s32\)]] = OpConstantI [[type_i64]], 5
; CHECK-MIR-DAG: [[source_language:%[0-9]+\:iid\(s32\)]] = OpConstantI [[type_i64]], 3
; CHECK-MIR-DAG: [[debug_info_version:%[0-9]+\:iid\(s32\)]] = OpConstantI [[type_i64]], 21
; CHECK-MIR-DAG: [[filename_str:%[0-9]+\:id\(s32\)]] = OpString 1094795567, 1094795585, 792805697, 1111638594, 1111638594, 1128481583, 1128481603, 1697596227, 1886216568, 1663985004, 0
; CHECK-MIR-DAG: [[filename_str:%[0-9]+\:id\(s32\)]] = OpString 1094795567, 1094795585, 792805697, 1111638594, 1111638594, 1128481583, 1128481603, {{1697596227|1700545347}}, 1886216568, 1663985004, 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the hash?

Copy link
Contributor Author

@bogner bogner Aug 26, 2024

Choose a reason for hiding this comment

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

Nope, much worse. This is ascii (or maybe utf-8) printed as decimal integers. 1697596227 is 0x652f4343, and 1700545347 is 0x655c4343 - so the real diff is that we now accept 5c ('\') in the second character as well as 2f ('/').

Copy link
Member

@michalpaszkowski michalpaszkowski left a comment

Choose a reason for hiding this comment

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

LGTM!

@bogner bogner merged commit b1e0589 into llvm:main Aug 26, 2024
11 checks passed
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.

5 participants