[PATCH] D127198: [AMDGPU][MC][GFX1013] Enable image_msaa_load

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added inline comments.


================
Comment at: llvm/test/MC/AMDGPU/gfx1011_supported.s:1
+// RUN: llvm-mc -arch=amdgcn -mcpu=gfx1011 -mattr=+wavefrontsize32,-wavefrontsize64 -show-encoding %s | FileCheck %s
+// RUN: llvm-mc -arch=amdgcn -mcpu=gfx1012 -mattr=+wavefrontsize32,-wavefrontsize64 -show-encoding %s | FileCheck %s
----------------
dp wrote:
> kosarev wrote:
> > dp wrote:
> > > kosarev wrote:
> > > > dp wrote:
> > > > > kosarev wrote:
> > > > > > Will it harm if we drop the `_supported` bit from the file name? +Same below.
> > > > > These tests are a bit special and I added the suffix to distinguish them from regular tests. We also have tests with `unsupported` suffix which serve the same purpose. I'm planning to add `supported` tests for other GFX9 and GFX10 subtargets.
> > > > > 
> > > > > Another option I see is to move all `supported` tests to a separate folder (e.g. `AMDGPU/subtargets`), in this case the `unsupported` suffix will be unnecessary. 
> > > > > 
> > > > > Any other ideas?
> > > > It seems so far we have been adding tests that make sure instructions are supported for particular targets without seeing them any special? Also, if this is a 'supported' kind of test, then what should we think are other positive encoding tests? Feels like this may be unnecessary complication.
> > > You gave me a better idea. I think the `supported` tests may be further simplified by removing encoding checks. Thus they will only check that the specified instructions are supported, i.e. may be assembled without errors. This will also make these tests run faster. And after this change they will be special.
> > > 
> > > If nobody likes the idea, I'll rename the tests as you suggested.
> > If we want to make sure the support for the instructions is there, then why wouldn't we also want to check if they use the right encodings?
> The question is where to stop. Instead of adding these `supported` tests we could have added RUN lines for missing subtargets to existing tests, but that would double or even triple test time according to my estimations. Note that we have 9 GFX10 subtargets with poor to none test coverage.
The point was, why where we have reasons to think the support for assembling instructions may be missed we shouldn't also assume the same applies to using the right encodings. And if it does apply, then the expected tests should be no special compared to what we already have.

To clarify, I don't suggest adding more RUN lines to those huge tests whose usefulness doesn't seem proportional to the amount of time they consume -- on top of that they make cases we are truly interested in less visible. The cases covered in the new tests do seem sufficient to me, and as long as we keep them this way, it should promise any problems in terms of testing times, shouldn't it?


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

https://reviews.llvm.org/D127198