Page MenuHomePhabricator

[AArch64][BFloat] add BFloat instruction support for AArch64
ClosedPublic

Authored by stuij on May 11 2020, 4:49 AM.

Details

Summary

Add support for lowering various BFloat related SelDAG nodes:

  • load/store (ldrh/strh)
  • concat
  • dup/duplane
  • bitconvert/bitcast
  • insert_subvector/insert_subreg

This patch is part of a series implementing the Bfloat16 extension of the
Armv8.6-a architecture, as detailed here:

https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/arm-architecture-developments-armv8-6-a

The bfloat type, and its properties are specified in the Arm Architecture
Reference Manual:

https://developer.arm.com/docs/ddi0487/latest/arm-architecture-reference-manual-armv8-for-armv8-a-architecture-profile

Diff Detail

Event Timeline

stuij created this revision.May 11 2020, 4:49 AM

Could you add some tests (unless they're added in a later patch)?

As an aside, it looks like we have a large amount of vector-related patterns in AArch64InstrInfo.td that are the same except that the types are different. It would be nice if we could reduce that by making use of foreach and/or multiclass, though I don't expect you to do that here.

stuij added a comment.May 13 2020, 6:30 AM

Could you add some tests (unless they're added in a later patch)?

Yes, thanks. Adding tests is the big todo for this patch.

stuij updated this revision to Diff 263733.May 13 2020, 8:13 AM

fix bug in ReplaceBITCASTResults

stuij updated this revision to Diff 265478.May 21 2020, 5:14 AM

added testcases, following fp16's lead

fpetrogalli requested changes to this revision.May 21 2020, 9:19 AM

HI @stuij ,

thank you for working on this. Please make sure that you use check-label and not just check when looking for the symbols of the functions. I have marked one place where this is happening. Also, please remove the tests that are checking the "kill" comments (one marked in the tests). Please make sure you fix it in all test cases before submitting.

Feel free to address also the two nits below if they make sense to you, but I won't hold my position on those. :)

Francesco

Nit1: I think you should make sure that nothing else than the instruction you are testing is generated, by invoking llc with -asm-verbose=0 and modifying the tests as follows:

define <4 x i16> @v4bf16_to_v4i16(float, <4 x bfloat> %a) nounwind {
; CHECK-LABEL: v4bf16_to_v4i16:
; CHECK-NEXT: mov v0.16b, v1.16b
; CHECK-NEXT: ret
entry:
  %1 = bitcast <4 x bfloat> %a to <4 x i16>
  ret <4 x i16> %1
}

Nit2: You mark some functions with #0, which is not defined anywhere, please remove it.

llvm/test/CodeGen/AArch64/bf16-vector-shuffle.ll
20

What are you testing here? These lines seem not necessary in terms of code generation. Please remove, here and in all other tests.

llvm/test/CodeGen/AArch64/bf16.ll
8

Please use check-label, not check, when testing symbols in this file. Also, please make sure you test the end of the function name by testing also the final :, as in // CHECK-LABEL: test_label:

Please fix all the tests in this file.

This revision now requires changes to proceed.May 21 2020, 9:19 AM
stuij edited the summary of this revision. (Show Details)May 22 2020, 3:13 AM
stuij marked 2 inline comments as done.May 22 2020, 8:31 AM

Thanks Francesco, that was enlightening, and great info to know. I took on all your suggestions, and changed the testing accordingly.

stuij updated this revision to Diff 265746.May 22 2020, 8:47 AM

making testing more robust as per review comments

fpetrogalli accepted this revision.May 26 2020, 10:11 AM

LGTM! Thank you.

Francesco

This revision is now accepted and ready to land.May 26 2020, 10:11 AM
stuij updated this revision to Diff 266522.May 27 2020, 6:53 AM

update commit text

This revision was automatically updated to reflect the committed changes.