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

Wed Jun 8 05:36:21 PDT 2022

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:
> > > > 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?

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

https://reviews.llvm.org/D127198