[PATCH] D103663: [AMDGPU] Add gfx1013 target

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon marked 8 inline comments as done.
bcahoon added inline comments.


================
Comment at: llvm/docs/AMDGPUUsage.rst:389
                                                     - xnack             scratch       - *pal-amdpal*
+     ``gfx1013``                 ``amdgcn``   dGPU  - cumode          - Absolute      - *rocm-amdhsa* *TBA*
+                                                    - wavefrontsize64   flat          - *pal-amdhsa*
----------------
foad wrote:
> Is it dGPU or APU?
> 
> Every other entry with `*TBA*` also has a `TODO::` message
It is APU.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPU.td:468
 
+def FeatureGFX10_AEncoding : SubtargetFeature<"gfx10_a-encoding",
+  "GFX10_AEncoding",
----------------
foad wrote:
> What is this new encoding? It doesn't seem to be used for anything.
Fixed this. The BVH raytracing instructions use the encoding. 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPU.td:1106
   [FeatureGFX10,
    FeatureGFX10_BEncoding,
    FeatureGFX10_3Insts,
----------------
rampitec wrote:
> gfx1030 should now include FeatureGFX10_AEncoding as well. 10_B is an extension above 10_A.
I had added FeatureGFX10_AEncoding as an Implies feature for FeatureGFX10_BEncoding in the previous patch.  But, I've changed the patch so that it's no longer an Implies feature.


================
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
 
----------------
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.


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

https://reviews.llvm.org/D103663