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

[Ilia Diachkov via Phabricator via llvm-commits]

iliya-diyachkov added inline comments.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVBuiltins.td:57
 def LoadStore : BuiltinGroup;
+def ExpectAssume : BuiltinGroup;
 
----------------
pmatos wrote:
> 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?
I'm not sure where this new ExpectAssume is actually used. If you are going to use ExpectAssume builtin in later version of the patch (or in the next patch), please add it later.


================
Comment at: llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp:860
+    Reqs.addCapability(SPIRV::Capability::ExpectAssumeKHR);
+    break;
   default:
----------------
pmatos wrote:
> 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?
The place looks good but we need to check ST.canUseExtension().

> Or is it possible that two different extensions provide the same capability?
I'm not sure but Reqs should work as a set and do not duplicate items.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157696