[PATCH] D157696: [SPIRV] Implement support for SPV_KHR_expect_assume

[Paulo Matos via Phabricator via llvm-commits]

pmatos added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVBuiltins.td:57
 def LoadStore : BuiltinGroup;
+def ExpectAssume : BuiltinGroup;
 
----------------
I have a question here... We have these instruction groups. I added these to builtins but I feel it's not the right thing to do. I get a warning during compilation because I didn't handle these instructions:
```
/home/pmatos/dev/llvm-project/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp:1933:11: warning: enumeration value 'ExpectAssume' not handled in switch [-Wswitch]
  switch (Call->Builtin->Group) {
          ^~~~~~~~~~~~~~~~~~~~
1 warning generated.
```

Should I create a group just for these instructions instead?


================
Comment at: llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:860
+    Reqs.addCapability(SPIRV::Capability::ExpectAssumeKHR);
+    break;
   default:
----------------
Other cases don't addExtensions here, just capabilities but if I don't then there's really no good place to add extensions. Would it make sense to have a loop to add extensions based on capabilities required? Or is it possible that two different extensions provide the same capability?


================
Comment at: llvm/test/CodeGen/SPIRV/assume.ll:2
+; RUN: llc -mtriple=spirv32-unknown-unknown < %s | FileCheck %s
+; RUN: llc -mtriple=spirv64-unknown-unknown < %s | FileCheck %s
+
----------------
I added testing on spirv64, I assume this makes sense.


================
Comment at: llvm/test/CodeGen/SPIRV/assume.ll:16
+}
\ No newline at end of file

----------------
I will make sure to add a new line in the next revision of the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157696/new/

https://reviews.llvm.org/D157696