bcahoon marked an inline comment as done. bcahoon added inline comments.
================ Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7341 + if (!Subtarget->hasGFX10_AEncoding()) + emitRemovedIntrinsicError(DAG, DL, Op.getValueType()); + ---------------- rampitec wrote: > return emitRemovedIntrinsicError(); I've changed this to return. Thanks for catching that. But, it returns a UNDEF value instead of SDValue() so that it doesn't crash. I can change the behavior if that's preferred. ================ 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: > 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. I've left the patch so that it doesn't crash. But, let me know if you think we should return false and crash, and I'll make that change. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103663/new/ https://reviews.llvm.org/D103663 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
