[Openmp-commits] [PATCH] D103663: [AMDGPU] Add gfx1013 target

[Stanislav Mekhanoshin via Phabricator via Openmp-commits]

rampitec added inline comments.


================
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:
> > 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.
> The diagnostic is marked as an error, so the compilation fails in that llc returns a non-zero return code. This mechanism is used in other places in the back-end to report similar types of errors. The alternative, if I understand correctly, is that a crash occurs with an error message that indicates that the bug is in LLVM (rather the the input source file).
We do not seem to be consistent here and return either undef or SDValue(), but as far as I can see we never continue selecting code though, like here in SIISelLowering and always return false from the AMDGPUInstructionSelector.


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

https://reviews.llvm.org/D103663