Fix support of SPV_INTEL_vector_compute#1176
Fix support of SPV_INTEL_vector_compute#1176AlexeySotkin merged 2 commits intoKhronosGroup:masterfrom
Conversation
The previous patch has fixed only case with 1-element vectors, however after closer look at the spec it became clear that SPV_INTEL_vector_compute actually allows any number of vector elements (capability VectorAnyINTEL), so this is the proper fix.
| !(Opts.isAllowedToUseExtension( | ||
| ExtensionID::SPV_INTEL_vector_compute) && | ||
| NumElemsInSrcVec == 1)) | ||
| !Opts.isAllowedToUseExtension( |
There was a problem hiding this comment.
In case if this extension is included we doesn't lowering any instructions with vector types.
Maybe to move this check just to the beginning of this method?
There was a problem hiding this comment.
I was just thinking that maybe this pass will lower not only vector instructions one day, so I left it here. Do you think it is possible?
There was a problem hiding this comment.
I think that we might rename this concrete method to use it for non standard vectors lowering only. And to write new functions for lowering other non supported patterns. It would make these changes atomic to each other.
There was a problem hiding this comment.
Ok, I've moved check to the beginning.
| // in pure SPIR-V number of components, there is no need to do anything in | ||
| // case SPV_INTEL_vector_compute is enabled. | ||
| if (Opts.isAllowedToUseExtension(ExtensionID::SPV_INTEL_vector_compute)) | ||
| return PreservedAnalyses::all(); |
There was a problem hiding this comment.
It is out of scope of this patch, but it is strange that we return PreservedAnalyses, because it is not an analysis pass AFAIK.
There was a problem hiding this comment.
I was following the logic that was made here before. I'm a newbie in passes that run through new passmanager. Probably @KornevNikita can help to understand it better once returns from vacation.
There was a problem hiding this comment.
I suppose current method better to return just bool "changed or not".
In main run method PreservedAnalyses should be created depending on changed value.
The previous patch has fixed only case with 1-element vectors, however after closer look at the spec it became clear that SPV_INTEL_vector_compute actually allows any number of vector elements (capability VectorAnyINTEL), so this is the proper fix.
The previous patch has fixed only case with 1-element vectors, however after closer look at the spec it became clear that SPV_INTEL_vector_compute actually allows any number of vector elements (capability VectorAnyINTEL), so this is the proper fix.
The previous patch has fixed only case with 1-element vectors, however after closer look at the spec it became clear that SPV_INTEL_vector_compute actually allows any number of vector elements (capability VectorAnyINTEL), so this is the proper fix.
The previous patch has fixed only case with 1-element vectors, however after closer look at the spec it became clear that SPV_INTEL_vector_compute actually allows any number of vector elements (capability VectorAnyINTEL), so this is the proper fix.
The previous patch has fixed only case with 1-element vectors, however after closer look at the spec it became clear that SPV_INTEL_vector_compute actually allows any number of vector elements (capability VectorAnyINTEL), so this is the proper fix.
4 participants
The previous patch has fixed only case with 1-element vectors, however
after closer look at the spec it became clear that
SPV_INTEL_vector_compute actually allows any number of vector elements
(capability VectorAnyINTEL), so this is the proper fix.