This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Predicate bfloat16 load patterns with HasBF16
ClosedPublic

Authored by kmclaughlin on Jun 24 2020, 7:17 AM.

Diff Detail

Event Timeline

kmclaughlin created this revision.Jun 24 2020, 7:17 AM
Herald added a project: Restricted Project. · View Herald Transcript
kmclaughlin added a reviewer: fpetrogalli.
  • Added an assert in performLDNT1Combine to check hasBF16() is true where the type is a bfloat16
fpetrogalli requested changes to this revision.Jun 24 2020, 12:47 PM
fpetrogalli added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12076–12077

No need to set up a variable when it is used only once. Please move it inside the assertion.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1554

[HasBF16, HasSVE], same for the Predicates below.

llvm/test/CodeGen/AArch64/sve-intrinsics-ld1-addressing-mode-reg-imm.ll
1

Nit: as I mentioned in https://reviews.llvm.org/D82492#inline-758743, maybe it is worth extracting the bf16 tests in a separate *-bfloat.ll file as we have done for the ACLEs.

This revision now requires changes to proceed.Jun 24 2020, 12:47 PM
kmclaughlin marked 3 inline comments as done.
  • Moved hasBF16() check into the assert in performLDNT1Combine
  • Added HasSVE to the Predicates
  • Added a function attribute for the bfloat tests
llvm/test/CodeGen/AArch64/sve-intrinsics-ld1-addressing-mode-reg-imm.ll
1

I've added a function attribute for the bfloat16 functions, the same as the tests added in D82429

c-rhodes added inline comments.Jun 25 2020, 5:39 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12078–12080

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)");
fpetrogalli accepted this revision.Jun 25 2020, 7:21 AM

LGTM! :)

This revision is now accepted and ready to land.Jun 25 2020, 7:21 AM
kmclaughlin marked an inline comment as done.Jun 26 2020, 2:46 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen added inline comments.Jun 26 2020, 3:17 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12078–12080

I think you should return SDValue() here instead.

sdesmalen added inline comments.Jun 26 2020, 3:27 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12078–12080

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.

kmclaughlin marked an inline comment as done.Jun 26 2020, 11:13 AM
kmclaughlin added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12078–12080

Thanks for explaining this @sdesmalen, I've removed these asserts and replaced them with return SDValue() as suggested in https://reviews.llvm.org/rG6b313f198c95218b953f2c992f702f178c61cd1d