This is an archive of the discontinued LLVM Phabricator instance.

[SPIR-V] Remove -opaque-pointers=0 from LITs, fixes for opaque pointers support
ClosedPublic

Authored by mpaszkowski on Jul 23 2023, 2:33 AM.

Details

Summary

This patch removes the -opaque-pointers=0 flag from all* the remaining tests. Some significant changes to the SPIRVGlobalRegistry, SPIRVDupliactesTracker, Intrinsics were necessary -- most notably the introduction of spv_assign_ptr_type intrinsic for tracking the underlying type of pointers.

*The following relevant tests are removed by this patch (they still require additional adjustments and fixes in the backend, will reland in subsequent patches):

  • CodeGen/SPIRV/EnqueueEmptyKernel.ll
  • CodeGen/SPIRV/OpenCL/basic/vstore_private.ll
  • CodeGen/SPIRV/OpenCL/execute_block.ll
  • CodeGen/SPIRV/half_no_extension.ll
  • CodeGen/SPIRV/transcoding/BuildNDRange_2.ll
  • CodeGen/SPIRV/transcoding/block_w_struct_return.ll
  • CodeGen/SPIRV/transcoding/enqueue_kernel.ll
  • CodeGen/SPIRV/transcoding/global_block.ll

In the upcoming week I will do best to recover the CTS testing pipeline. This requires generating of opaque IR (opaque pointers, target extension type, ...) from Intel Graphics Compiler based on LLVM 14. I expect the CTS pass rate to drop significantly.

In the meantime, I am preparing a patch dropping support for typed pointers (removing references to APIs), OpenCL/SPIR-V builtin opaque types, and any other "dead" parts of code. This patch will also partially tackle the issues discovered in the LITs from the list above and failing CTS tests.

There are still known issues which might be now easier to work on since we no longer need to support typed pointers:

  • issues with lowering enqueue, ndrange builtins
  • issues with lowering some structs in opaque pointer mode
  • duplicate getOrCreateSPIRVPointerType methods in SPIRVGlobalRegistry
  • opaque pointer handling in SPIRVDuplicatesTracker may require major redesign

I hope to discuss some of these issues during the SPIR-V Backend WG sync on Thursday.

Diff Detail

Event Timeline

mpaszkowski created this revision.Jul 23 2023, 2:33 AM
mpaszkowski requested review of this revision.Jul 23 2023, 2:33 AM
pmatos added a subscriber: pmatos.Jul 25 2023, 5:09 AM

In general it looks good. Please keep failed tests to view the progress of development and allow others to troubleshoot.

llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.h
27

Is it used inside SPIRVDuplicatesTracker?

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
933

Does it affect DT?

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
283 ↗(On Diff #543264)

It might be better to keep both interfaces removing duplication inside them.

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
1487

Does it always have a pointer type?

bogner added a subscriber: bogner.Aug 7 2023, 1:58 PM
bogner added inline comments.
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
751

isOpaquePointerTy is deprecated and just returns isPointerTy as of 2ea5aa1c96cf, so the else branch here (and in other similar cases) is unreachable and can be removed.

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.

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.

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.

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?

mpaszkowski added a comment.EditedSep 17 2023, 1:56 AM

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?

Hi @iliya-diyachkov! You are right about this test, I have restored it and rebased the patch. I will work on the linked lists later. When it comes to structs, I have a patch with additional tests for which I will create an independent review. Do you think I could continue with this patch in the current form? This will allow us to address next issues in smaller patches and should unblock Nathan's changes.

iliya-diyachkov accepted this revision.Sep 17 2023, 4:27 PM

Thanks Michal, it look good to me.

This revision is now accepted and ready to land.Sep 17 2023, 4:27 PM
llvm/test/CodeGen/SPIRV/transcoding/get_image_num_mip_levels.ll