- 
                Notifications
    
You must be signed in to change notification settings  - Fork 365
 
Enable metal tests #8446
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
Enable metal tests #8446
Conversation
Enables all tests/metal/ tests that can be easily enabled. These tests were not originally designed as render tests; they are generally being enabled for pipecleaning purposes, and will not be rigorously testing the corresponding funcitonality. Where they cannot be enabled as render tests, and metallib wasn't enabled, where possible metallib was enabled instead. Fixes shader-slang#7892
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall.
I left a few comments.
Note that whenever we can run the runtime test for Metal, it is also expected to run with Vulkan with the exactly same result.
        
          
                tests/metal/barrier.slang
              
                Outdated
          
        
      | @@ -1,18 +1,24 @@ | |||
| //TEST:SIMPLE(filecheck=CHECK): -target metal | |||
| //TEST:SIMPLE(filecheck=CHECK-ASM): -target metallib | |||
| // NOTE: Test not designed for compute, executing for pipecleaning only | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this comment.
We have a lot of tests that are just too simple to be useful but works as smoke tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood.
It worries me a bit how redundant/duplicative some of our testing is. I've seem a (smoke) argument to //TEST in some places.. is that intended to indicate these sorts of scenarios, or does it serve a different purpose?
| 
           Two remaining failures: test-macos-release-clang-aarch64 / test-slang 
 test-windows-release-cl-x86_64-gpu / test-slang 
  | 
    
Make the M1 runners happy by allowing a wider range of floating point values. (Roughly corresponds to what's documented in the Metal spec.)
| 
           I'm not able to repro the  CI is giving the following error: Not sure if this is a CI issue or a local issue. I do have VulkanSDK installed locally, so I guess theoretically that might be interfering with the slang in   | 
    
void* nullptr is not handled by glsl
| 
           @jkwak-work mind taking another look? I think all of the issues have been dealt with now.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks for also enabling some of vk tests as well.
Enables all tests/metal/ tests that can be easily enabled.
These tests were not originally designed as render tests; they are generally being enabled for pipecleaning purposes, and will not be rigorously testing the corresponding funcitonality.
Where they cannot be enabled as render tests, and a metallib test wasn't already enabled, a metallib test was enabled instead (where possible).
Fixes #7892