[PATCH] D125992: [AMDGPU] gfx11 FLAT Instructions

Tue May 24 09:34:16 PDT 2022

kosarev added inline comments.

================
Comment at: llvm/test/MC/AMDGPU/flat-gfx11.s:5
+// RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1030 2>&1 %s | FileCheck --check-prefix=VI-GFX9_10-ERR --implicit-check-not=error: %s
+// RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1100 -show-encoding %s | FileCheck --check-prefix=GFX11 %s
+
----------------
rampitec wrote:
> sebastian-ne wrote:
> > rampitec wrote:
> > > Joe_Nash wrote:
> > > > rampitec wrote:
> > > > > Ditto 'not'
> > > > This test contains positive tests for gfx11, negative tests for gfx11, and negative tests for previous architectures. I it enables detection of errors with all of those. Is that ok to land as is?
> > > It has positive tests, but all the run lines prepended with 'not'. This is not right. I believe GFX11 run line shall not have it.
> > There are other files that have positive and negative tests in them where all run lines are prepended with 'not', e.g. sop2.s, vop1.s, vop3.s and trap.s, so this is not a new pattern.
> > 
> > A negative gfx11 test in this file is e.g.
> > ```
> > flat_load_b32 v1, v[3:4], s[0:1]
> > // GFX11-ERR: error: invalid operand for instruction
> > ```
> This is yet another problem. You have GFX11-ERR labels in this file, but no run line with this check. Move negative checks into gfx11-err.s and remove the 'not'. As a side note, it is unfortunate that smem.s does that.
For a large change like this, maybe it wouldn't be a terrible crime to follow the current conventions and address that already-existing problem separately?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125992