In D156049#4641370, @iliya-diyachkov wrote:

Sure, I have made this patch a bit smaller. Still all the fixes are present, but I have kept all the LIT names the same and kept them in the same directories. Renaming, moving, and merging of tests will come in a separate patch.

Thanks! What about removed tests, I'm not sure if we have an analogue of linked-list.ll (with check of OpTypeOpaque). If it's true, is it possible to write such a test?

@sudonatalie Thank you for the patch! LGTM!

In D156049#4636408, @iliya-diyachkov wrote:

Sorry for delay with the answer. It looks good in general.

Some tests look deleted and some look newly created, but they are just moved/renamed with many changes inside. Can we split these operations? I.e. make changes related to opaque pointers and builtin types in this patch and rename the tests in a separate patch. It will save the history of changes in the tests.

In D155978#4639499, @Keenuts wrote:

In D155978#4639094, @mpaszkowski wrote:

@Keenuts Hi Nathan, thanks for the patch! I agree with your approach and I think that this solution despite being a "hack" seems to be the most straightforward. Eventual differences could be handled in the backend. I would be open for other solutions in the future (for example in case other targets would also stumble upon a similar dilemma), but this looks like a completely valid approach to me.

Thanks!
Do you know how we can get this merged? (we have no commit access on our end).

Rebased on top of @pmatos patch, added CLI option for SPV_INTEL_optnone, and moved the SPV_INTEL_optnone test to /extensions/. @iliya-diyachkov: Do you think we could have the patch merged now?

@Keenuts Hi Nathan, thanks for the patch! I agree with your approach and I think that this solution despite being a "hack" seems to be the most straightforward. Eventual differences could be handled in the backend. I would be open for other solutions in the future (for example in case other targets would also stumble upon a similar dilemma), but this looks like a completely valid approach to me.

I will rebase the patch on top of @pmatos change adding SPV_INTEL_optnone by the end of the day.

In D156297#4534419, @pmatos wrote:

This is a draft patch - I want to discuss somethings inline.

The output for optnone.ll is:

bin/llc -O0 -mtriple=spirv32-unknown-unknown ../llvm/test/CodeGen/SPIRV/optnone.ll -o -
	OpCapability Kernel
	OpCapability Addresses
	OpCapability Linkage
	%1 = OpExtInstImport "OpenCL.std"
	OpMemoryModel Physical32 OpenCL
	OpExecutionMode %4 ContractionOff
	OpSource Unknown 0
	OpName %4 "_Z3foov"
	OpDecorate %4 LinkageAttributes "_Z3foov" Export
	%2 = OpTypeVoid
	%3 = OpTypeFunction %2
	OpCapability OptNoneINTEL               ; -- Begin function _Z3foov
	OpExtension "�" "�" "�" "�" "�" "�"
	%4 = OpFunction %2 DontInline %3
	%5 = OpLabel
	OpReturn
	OpFunctionEnd

OpExtension emits some funky unicode characters, I feel like I am doing this incorrectly but similarly to what's being done in SPIRVAsmPrinter::outputGlobalRequirements() :

MCInst Inst;
    Inst.setOpcode(SPIRV::OpExtension);
    addStringImm(getSymbolicOperandMnemonic(
                     SPIRV::OperandCategory::ExtensionOperand, Ext),
                 Inst);
    outputMCInst(Inst);

The other issue is that afaiu the OpExtension declaractions need to come up the top of the module. How is this organized in the backend so that I can emit these at the top of the module? Do I need to add these to the module requirements somehow?

Thanks for catching the typo!

In D156231#4531851, @pmatos wrote:

The site: https://spirv-testing.khronos.org/staging/
is not working at the moment, right? At least, this phab diff is not present there.

New final revision can be found here: https://reviews.llvm.org/D156049

Still WIP, but CTS basic/* tests in both opaque pointer mode and typed pointer mode are now fixed. We still have failures in CTS printf/* tests.

There are some new regressions after this individual patch, we are hitting an assert in SPIRVInstructionSelector for example in integer_ops / long_logic. Full testing results: https://spirv-testing.khronos.org/job/142/

Added assert(!MBB.empty() && "MBB is empty!"). This should make it easier to spot similar issues in the future.

Updated the commit with a fix to the failing OpenCL CTS test and added one new LIT test.

Let's wait for the CTS testing results before pushing, but the patch LGTM! Thank you @iliya-diyachkov!

In D145137#4182622, @arsenm wrote:

Why is this not changing any legalizer rules?

@iliya-diyachkov I checked the half / vstore_half_rtn test manually and it seems to work just fine. Looks like we just got one flaky run for some reason.

Thanks @iliya-diyachkov! Fixed!

Fixed formatting

There still remains a large number of LIT tests with pointers-to-opaque-structs representing OpenCL builtin types. I would suggest to do either one of these things in the next patch:

Fixed issues

Thank you! LGTM!

Fixed formatting

Thanks @arsenm and @iliya-diyachkov for your comments! I have created a new revision https://reviews.llvm.org/D143107 which provides a solution that is compatible with the Khronos SPIR-V Translator and should support nested aggregates. I will close this one.

One (more important) note, only the last error "Element '1': conversion from 'const llvm::SPIRV::FPRoundingMode::FPRoundingMode' to 'unsigned int' requires a narrowing conversion" shows up during compilation and causes it to break. All the other errors show up only in IntelliSense in Visual Studio. This seems to be a known issue with Visual Studio.

@iliya-diyachkov I updated Visual Studio and there seems to be many more problems with building with MSVC (table below). There are no warnings in SPIRVUtils.cpp.

@iliya-diyachkov Thanks! I will check with MSVC.

Full runtime testing results are now available: https://spirv-testing.khronos.org/job/82/. I will commit this patch after D142061.

@nikic I have created the issue for tracking the SPIR-V backend LIT tests with -opaque-pointers=0 here: https://github.com/llvm/llvm-project/issues/60133

Gentle ping!

@arsenm Thanks! I uploaded a new updated patch.

Thank you for the patch @iliya-diyachkov! The runtime testing results are now available. LGTM!