Add OpCooperativeMatrixApplyFunctionINTEL instruction #2214
Conversation
MrSidims
changed the title
MrSidims
force-pushed
the
support_joint_matrix_apply_function
branch
from
33691b7
to
95be8e6
Compare
There was a problem hiding this comment.
Wouldn't it be good to minimize test case to actually containing only necessary's lines for a test?
There was a problem hiding this comment.
In general I agree with this, and that is the way to add 90%+ of the tests. Yet ideally for language features we should have tests be almost end-to-end'ish (or Integration'ish), see test/transcoding/*.cl tests, unfortunately sycl is not yet fully unstreamed to llvm.org. So the closest thing until then is to generate IR from some sycl test and use it as an input to the translator.
Why I'm thinking, that language features should be tested like this? Practice showed up, that for example for matrices having just calls to __spirv_JointMatrixLoadINTEL(A), (B), (C) followed by __spirv_JointMatrixMadINTEL(A, B, C) tests almost nothing as the compiler will generate extra code during optimizations that will affect translation (prime example mem2reg pass inserting phi instructions selecting matrices). Counter argument to this is that such cases should be tested in the appropriate repository, where language APIs are being added, but it won't work for features under active development, as tests in those repositories won't be enabled until all components are ready. And trust me, you don't want to harry with fixes for issues coming from the real-world code, that a synthetic test doesn't cover amid release period, especially taking into the account the fact, that reverse translator changes should be adopted across consumers.
There was a problem hiding this comment.
And for this very example - I'm not trusting myself to write LLVM IR that have an external function (acting like a SPIR-V instruction), accepting a function object in a reference wrapper.
There was a problem hiding this comment.
And btw, the test is quite minimized, all metadata, attributes and other not needed stuff is removed
There was a problem hiding this comment.
In general I agree with this, and that is the way to add 90%+ of the tests. Yet ideally for language features we should have tests be almost end-to-end'ish (or Integration'ish), see test/transcoding/*.cl tests, unfortunately sycl is not yet fully unstreamed to llvm.org. So the closest thing until then is to generate IR from some sycl test and use it as an input to the translator.
And it's ok to use frontend to get IR. But the next steps from that point is to strip everything which is unneeded and blurs the essence of the test.
Minimization here have a simple goal - to separate things which are necessary to provide a functionality and nothing more. So no @llvm.assume or @llvm.lifetime.*, !0, func attr etc. - clear minimal IR needed for feature. Then, when you want to extend a feature or correct a bug one year from that point - on first sight you know what feature looked like when it was implemented and what's needed to fix.
The problem with using straight forward outputs from frontend is that after some time the implementation in frontend will change but the IR version of the test here assumes that it's unchanged - frozen in time. Recompilation of a test issue from frontend will be impossible (because file doesn't exist or is renamed) or it will yield different IR. Is it test still useful and correct or it's not? Clear to read? This is what happened with FPGA_memory_attributes tests - which were implemented few years ago.
There was a problem hiding this comment.
We could run opt -S -passes=metarenamer < reduced.ll > reduced2.ll on it in order to get rid off of burdened names like %arrayidx.i29.i.i.i.i.
There was a problem hiding this comment.
It will change, but it doesn't mean, that the current output should be untested. And at this point of time for this very feature the only place, where it can be tested is llvm-spirv.
There was a problem hiding this comment.
I just think that the feature test should be focused only on thing which is implemented feature - nothing more and nothing less. And implemented code should be tested thoroughly. IMO. If the whole sequence of features need to be tested and for singular frontend then it's some kind of integration test and should be treated differently than typical feature test.
There was a problem hiding this comment.
opt -S -passes=metarenamer < reduced.ll > reduced2.ll
this actually renames also useful things, like functions and structures
test/extensions/INTEL/SPV_INTEL_joint_matrix/cooperative_matrix_apply.ll
Show resolved
Hide resolved
What changes are planned to be made in spec? |
…eMatrixInvocationInstructionsINTEL capability
Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
MrSidims
force-pushed
the
support_joint_matrix_apply_function
branch
from
05c96fb
to
7682dd4
Compare
| @@ -78,6 +78,7 @@ enum InternalOp { | |||
| IOpMaskedScatterINTEL = 6429, | |||
| IOpJointMatrixGetElementCoordINTEL = 6440, | |||
| IOpCooperativeMatrixPrefetchINTEL = 6449, | |||
| IOpCooperativeMatrixApplyFunctionINTEL = 6448, | |||
There was a problem hiding this comment.
nit: Reorder to make 6448 come first?
…ange of CooperativeMatrixUse
This PR aims to introduce entities related to OpCooperativeMatrixApplyFunctionINTEL in llvm-spirv translator, according to https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc.
TODO update the spec