KhronosGroup / SPIRV-LLVM-Translator

A tool and a library for bi-directional translation between SPIR-V and LLVM IR
Other
479 stars 213 forks source link

test: Fix x86 tests to use -mtriple=x86_64-unknown-linux-gnu #2555

Closed mattst88 closed 3 months ago

mattst88 commented 5 months ago

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 commented 5 months ago

CLA assistant check
All committers have signed the CLA.

mattst88 commented 5 months ago

cc: @AlexeySotkin

mattst88 commented 5 months ago

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

Maybe @MrSidims can offer some advice.

mattst88 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Should I update the PR to do that?

Speculatively updated.

mattst88 commented 3 months ago

Does this look good to you, @MrSidims?