[PATCH] D103663: [AMDGPU] Add gfx1013 target

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4697
+  if (!ST.hasGFX10_AEncoding()) {
+    DiagnosticInfoUnsupported BadIntrin(B.getMF().getFunction(), "intrinsic not supported on subtarget",
+                                        MI.getDebugLoc());
----------------
80 chars per line.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4700
+    B.getMF().getFunction().getContext().diagnose(BadIntrin);
+    B.buildUndef(MI.getOperand(0));
+    MI.eraseFromParent();
----------------
Just return false like in other places.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7341
+    if (!Subtarget->hasGFX10_AEncoding())
+      emitRemovedIntrinsicError(DAG, DL, Op.getValueType());
+
----------------
return emitRemovedIntrinsicError();


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.intersect_ray.ll:4
+; RUN: llc -global-isel -march=amdgcn -mcpu=gfx1013 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: not llc -global-isel -march=amdgcn -mcpu=gfx1012 -verify-machineinstrs < %s -o /dev/null 2>&1 | FileCheck -check-prefix=ERR %s
 
----------------
not --crash llc ...


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.intersect_ray.ll:4
+; RUN: llc -global-isel -march=amdgcn -mcpu=gfx1013 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -global-isel -march=amdgcn -mcpu=gfx1012 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
 
----------------
bcahoon wrote:
> rampitec wrote:
> > rampitec wrote:
> > > foad wrote:
> > > > This test surely should not pass for gfx1012, since it does not have these instructions. And with your patch as written it should fail for gfx1013 too, since they are predicated on HasGFX10_BEncoding.
> > > > 
> > > > @rampitec any idea what is wrong here? Apparently the backend will happily generate image_bvh_intersect_ray instructions even for gfx900!
> > > Indeed. MIMG_IntersectRay has this:
> > > 
> > > ```
> > >   let SubtargetPredicate = HasGFX10_BEncoding,
> > >       AssemblerPredicate = HasGFX10_BEncoding,
> > > ```
> > > but apparently SubtargetPredicate did not work. It needs to be fixed.
> > > 
> > > gfx1012 does not have it, gfx1013 does though. That is what GFX10_A encoding is about, 10_B it has to be replaced with 10_A in BVH and MSAA load.
> > Image lowering and selection is not really done like everything else. For BVH it just lowers intrinsic to opcode. I think the easiest fix is to add to SIISelLowering.cpp where we lower Intrinsic::amdgcn_image_bvh_intersect_ray something like this:
> > 
> > 
> > ```
> >   if (!Subtarget->hasGFX10_AEncoding())
> >     report_fatal_error(
> >         "requested image instruction is not supported on this GPU");
> > ```
> I ended up using emitRemovedIntrinsicError, which uses DiagnosticInfoUnsupported. This way the failure isn't a crash dump.
> I ended up using emitRemovedIntrinsicError, which uses DiagnosticInfoUnsupported. This way the failure isn't a crash dump.

Diagnostics is a good thing, but we still have to fail the compilation.


================
Comment at: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.intersect_ray.ll:3
+; RUN: llc -march=amdgcn -mcpu=gfx1013 -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+; RUN: not llc -march=amdgcn -mcpu=gfx1012 -verify-machineinstrs < %s 2>&1 | FileCheck -check-prefix=ERR %s
 
----------------
not --crash llc


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

https://reviews.llvm.org/D103663