bcahoon marked an inline comment as done.
bcahoon added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPU.td:471
+ "true",
+ "Encoding format GFX10_A"
+>;
----------------
foad wrote:
> I realise you're just following the precedent set by GFX10_B, but is this
> terminology actually used in any documentation anywhere? And if not could we
> describe it a little better here?
I changed the description to be specific w.r.t what the target feature enables.
================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7341
+ if (!Subtarget->hasGFX10_AEncoding())
+ emitRemovedIntrinsicError(DAG, DL, Op.getValueType());
+
----------------
foad wrote:
> bcahoon wrote:
> > 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.
> Personally I would follow all the existing precedents and "return
> emitRemovedIntrinsicError(...)". I don't see any value in deliberately trying
> to make the compiler crash harder.
The handling of diagnostic errors is a little inconsistent in ISelLowering.
Sometimes SDValue() is returned and other times it's Undef. I have it return
SDValue() so that the failure mode is consistent with how GlobaISel handles
these intrinsics. It's probably worth a discussion to decide how best to handle
diagnostic errors. I'm happy to submit a follow-on patch as needed.
As an aside, return emitRemovedIntrinsicError() isn't enough because the
intrinsic has both a return value and a chain edge. So, something like return
DAG.getMergeValues({emitRemovedIntrinsicError(), Op.getValue(0)}, DL) is needed.
Repository:
rG LLVM Github Monorepo
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
