[PATCH] D157696: [SPIRV] Implement support for SPV_KHR_expect_assume
Paulo Matos via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 11 03:09:00 PDT 2023
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
More information about the llvm-commits
mailing list