[PATCH] D134354: [AMDGPU][GlobalISel] Support mad/fma_mix selection

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/mad-mix.ll:1-4
+; RUN: llc -global-isel -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -show-mc-encoding < %s | FileCheck -enable-var-scope -check-prefixes=GCN,GFX900,GFX9 %s
+; RUN: llc -global-isel -march=amdgcn -mcpu=gfx906 -verify-machineinstrs -show-mc-encoding < %s | FileCheck -enable-var-scope -check-prefixes=GCN,GFX906,GFX9 %s
+; RUN: llc -global-isel -march=amdgcn -mcpu=fiji -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefixes=GCN,CIVI,VI %s
+; RUN: llc -global-isel -march=amdgcn -mcpu=hawaii -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefixes=GCN,CIVI,CI %s
----------------
arsenm wrote:
> Pierre-vh wrote:
> > arsenm wrote:
> > > This is a copy pasted version of the existing test. I'd assume they can share the same runlines in the same file
> > Some run lines are different unfortunately, see FIXMEs in the test
> I'd work to eliminate those differences. If it turns out to be difficult, I would probably switch to generated checks and have them share the same file
The remaining difference fall in the following categories:
 - isCanonicalized has different behaviour between GISel/DAG, so there's v_mac instead of v_mad in a few places since SIFoldOperand doesn't fold
   - there's also v_madak_f32 that's no longer present, I think it's the same cause but I haven't looked into it yet
 - op_sel is on the second v_mad_mix instead of the first, seems like a harmless difference due to how DAG/GISel work?
 - A G_LSHR + G_SHUFFLEVECTOR (1,0) pair isn't folded out. I think those operations negate each other, perhaps a combine should be added for that?
 - Some unfinished things like -[v0| not being picked up yet.

The last 2 definitely have to be fixed, I'll look into them ASAP, but are the first 2 important as well? I'm not sure of what to do with `isCanonicalized`, is there a place where I can find the list of operations that should go in there?


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:2526-2530
+  /// Report the maximum number of temporary operands needed by the predicate
+  /// matcher.
+  unsigned countRendererFns() const override {
+    return InsnMatcher->countRendererFns();
+  }
----------------
arsenm wrote:
> This looks unrelated
Actually it isn't, it looks like the FMA/MAD patterns expose a bug in GISel. Without that there's a crash (segfault) in `executeMatchTable` because the number of renderer fns is incorrectly reported and it doesn't allocate enough entries in the vector that holds them. It seems like we rarely go above 2 renderers but here there's 4 IIRC 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134354