Skip to content

Conversation

@mattst88
Copy link
Contributor

@mattst88 mattst88 commented May 6, 2024

These tests fail on 32-bit x86:

x86 ~/SPIRV-LLVM-Translator # lit -vv -j128 build/test/DebugInfo/X86/ 
[...]
********************
Failed Tests (7):
  LLVM_SPIRV :: DebugInfo/X86/dbg-declare-alloca.ll
  LLVM_SPIRV :: DebugInfo/X86/dbg-declare-arg.ll
  LLVM_SPIRV :: DebugInfo/X86/dbg-value-const-byref.ll
  LLVM_SPIRV :: DebugInfo/X86/dw_op_minus_direct.ll
  LLVM_SPIRV :: DebugInfo/X86/dwarf-aranges-no-dwarf-labels.ll
  LLVM_SPIRV :: DebugInfo/X86/frame-register.ll
  LLVM_SPIRV :: DebugInfo/X86/this-stack_value.ll


Testing Time: 0.70s

Total Discovered Tests: 87
  Unsupported:  1 (1.15%)
  Passed     : 79 (90.80%)
  Failed     :  7 (8.05%)

The causes are all pretty simple differences from the x86-64 expectations in the tests today. E.g.:

+ /usr/lib/llvm/18/bin/FileCheck /root/SPIRV-LLVM-Translator/test/DebugInfo/X86/frame-register.ll
/root/SPIRV-LLVM-Translator/test/DebugInfo/X86/frame-register.ll:15:15: error: CHECK-NEXT: expected string not found in input
; CHECK-NEXT: DW_AT_location [DW_FORM_exprloc] (DW_OP_fbreg +0)
              ^
<stdin>:26:28: note: scanning from here
0x0000003b: DW_TAG_variable [3] (0x00000026)
                           ^
<stdin>:27:2: note: possible intended match here
 DW_AT_location [DW_FORM_exprloc] (DW_OP_fbreg +4)
 ^

Or

/root/SPIRV-LLVM-Translator/test/DebugInfo/X86/dbg-value-const-byref.ll:39:15: error: CHECK-NEXT: expected string not found in input
; CHECK-NEXT: [[C2]], [[R1:0x.*]]): DW_OP_reg0 RAX
              ^
<stdin>:64:43: note: scanning from here
 [0x0000000f, 0x00000014): DW_OP_consts +7
                                          ^
<stdin>:64:43: note: with "C2" equal to "0x00000014"
 [0x0000000f, 0x00000014): DW_OP_consts +7
                                          ^
<stdin>:65:16: note: possible intended match here
 [0x00000014, 0x00000018): DW_OP_reg0 EAX
               ^

With these expectation updates, the test suite passes on x86-32:

x86 ~/SPIRV-LLVM-Translator # lit -vv -j128 build/test/DebugInfo/X86/ 
[...]
Testing Time: 0.73s

Total Discovered Tests: 87
  Unsupported:  1 (1.15%)
  Passed     : 86 (98.85%)

However, I do not know how to conditionalize the CHECK/CHECK-NEXT/DWARF/DWARF-NEXT statements. I found that some tests in LLVM use X86/X86-NEXT/X64/X64-NEXT statements, but I do not understand things well enough to use them in these tests. This is where I need help.

@CLAassistant
Copy link

CLAassistant commented May 6, 2024

CLA assistant check
All committers have signed the CLA.

@mattst88
Copy link
Contributor Author

mattst88 commented May 7, 2024

cc: @AlexeySotkin

@mattst88
Copy link
Contributor Author

mattst88 commented May 8, 2024

Looks like Alexey is no longer working on SPIR-V.

Maybe @MrSidims can offer some advice.

@mattst88
Copy link
Contributor Author

I talked with Alexey and he explained how this works to me and pointed me to https://llvm.org/docs/CommandGuide/FileCheck.html#the-filecheck-check-prefix-option

Now that I understand a little bit better, I think I've recognized what the real problem is: these tests compile some .ll files with target triple = spir64-unknown-unknown and check for some x86-64-specifics in the resulting output, but there's no connection between spir64-unknown-unknown and x86-64.

AFAIU, spir64-unknown-unknown is SPIR-V targets a platform with 64-bit pointers and spir-unknown-unknown targets a platform with 32-bit pointers. I assume you can compile -mtriple=spir64-unknown-unknown for any CPU architecture LLVM supports, so checking for assembly instructions for a particular CPU isn't reliable.

Does this make sense?

@MrSidims
Copy link
Contributor

@mattst88 thanks for the contribution! We can either put something like REQUIRES: X86-64 or create a check logic depending on the architecture. I just returned from a vacation and will take a look throughout the day at the problem and suggest the appropriate changes.

@MrSidims
Copy link
Contributor

MrSidims commented May 13, 2024

@mattst88 agree, mtriple should be not %triple in this case. Instead it should be either x86_64-unknown-linux-gnu with the CHECK lines remain to be as is or instead RUN strings be duplicated having both 32 and 64 bit triples with the appropriate FileChecks to be FileCheck %s --check-prefixes=CHECK-x86-64 and FileCheck %s --check-prefixes=CHECK-X86-32.

I would ask you to just set triple -mtriple=x86_64-unknown-linux-gnu in this PR and if necessary we extend the testing on our own.

@mattst88
Copy link
Contributor Author

mattst88 commented Jun 7, 2024

The updated PR works for me on SPIRV-LLVM-Translator 17 and 18.1.

It was generated by

sed -i -e 's/%triple/x86_64-unknown-linux-gnu/' \
        test/DebugInfo/X86/dbg-declare-alloca.ll \
        test/DebugInfo/X86/dbg-declare-arg.ll \
        test/DebugInfo/X86/dbg-value-const-byref.ll \
        test/DebugInfo/X86/dw_op_minus_direct.ll \
        test/DebugInfo/X86/dwarf-aranges-no-dwarf-labels.ll \
        test/DebugInfo/X86/frame-register.ll \
        test/DebugInfo/X86/this-stack_value.ll

@mattst88
Copy link
Contributor Author

mattst88 commented Jun 7, 2024

That was enough to make the whole test suite pass on x86-32, but there are still failures when running it on other platforms like aarch64. Looks like sed -i -e 's/%triple/x86_64-unknown-linux-gnu/' test/DebugInfo/X86/*.ll is enough to make at least all of test/DebugInfo/X86 pass on aarch64.

Should I update the PR to do that?

@mattst88
Copy link
Contributor Author

mattst88 commented Jun 7, 2024

Should I update the PR to do that?

Speculatively updated.

... when test expects x86_64-specific results.
@mattst88 mattst88 changed the title [WIP] test: Fix test expectations on x86-32 test: Fix x86 tests to use -mtriple=x86_64-unknown-linux-gnu Jun 7, 2024
@mattst88
Copy link
Contributor Author

Does this look good to you, @MrSidims?

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

LGTM

@MrSidims MrSidims merged commit 681f027 into KhronosGroup:main Jun 26, 2024
@mattst88 mattst88 deleted the fix-tests-on-x86 branch September 17, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants