[PATCH] D82464: [AArch64][SVE] Predicate bfloat16 load patterns with HasBF16

Kerry McLaughlin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 26 11:29:49 PDT 2020


kmclaughlin marked an inline comment as done.
kmclaughlin added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12078-12080
+      (VT != MVT::nxv8bf16 ||
+       static_cast<const AArch64Subtarget &>(DAG.getSubtarget()).hasBF16()) &&
+      "Unsupported type (BF16)");
----------------
sdesmalen wrote:
> sdesmalen wrote:
> > c-rhodes wrote:
> > > nit: maybe it's just me but this hurts my head, can we not just write:
> > > ```  if (VT == MVT::nxv8bf16)
> > >     assert(
> > >         static_cast<const AArch64Subtarget &>(DAG.getSubtarget()).hasBF16() &&
> > >         "Unsupported type (BF16)");```
> > I think you should `return SDValue()` here instead.
> Some reasoning to my above statement:
> Without asserts enabled, this will just continue with the code below. It's better to let the compiler fail with the standard error that it cannot select this intrinsic, regardless of whether asserts are enabled or not.
Thanks for explaining this @sdesmalen, I've removed these asserts and replaced them with `return SDValue()` as suggested in https://reviews.llvm.org/rG6b313f198c95218b953f2c992f702f178c61cd1d


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82464





More information about the llvm-commits mailing list