This is an archive of the discontinued LLVM Phabricator instance.

[X86][BF16] Make backend type bf16 to follow the psABI
ClosedPublic

Authored by pengfei on Jul 30 2022, 11:07 PM.

Details

Summary

X86 psABI has updated to support __bf16 type, the ABI of which is the
same as FP16. See https://discourse.llvm.org/t/patch-add-optional-bfloat16-support/63149

This is an alternative of D129858, which has less code modification and
supports the vector type as well.

Diff Detail

Event Timeline

pengfei created this revision.Jul 30 2022, 11:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2022, 11:07 PM
pengfei requested review of this revision.Jul 30 2022, 11:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2022, 11:07 PM

Ping @bkramer. Together with D130964 and D131147, I think the change in BF16 ABI won't affect your scenarios. Let's speed up the review process and make them to be merged into LLVM 15.0 in time. So that we won't fall into the trap like #56854 again.

Ping @craig.topper. This approach is inspired by your comments in D107082. Can you help to have a look? I want to land it ASAP to catch up LLVM 15.0 so that we won't have ABI compatibility issues in future for compiler-rt.

craig.topper added inline comments.Aug 4 2022, 8:27 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
2439

What does it mean to Soften a vector?

craig.topper added inline comments.Aug 4 2022, 8:28 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
2439

It doesn't appear in the switch in TargetLoweringBase::computeRegisterProperties right after the call to getPreferredVectorAction

What's the status of the GlobalIsel implementation for X86? Do we also need to update that to support the new ABI?

pengfei updated this revision to Diff 450228.Aug 4 2022, 10:38 PM

Remove TypeSoftenFloat for vector action.

What's the status of the GlobalIsel implementation for X86? Do we also need to update that to support the new ABI?

AFAIK, GlobalIsel is not complete supported on X86. I don't know any user uses it.
So I think it's OK to not support ATM. Besides, we don't support for half either: https://godbolt.org/z/rd54zs9rr

llvm/lib/Target/X86/X86ISelLowering.cpp
2439

Good question. I think I tried this way but found it doesn't work currently and there're lots work to do to go this way. I must forget to delete it.

pengfei updated this revision to Diff 450229.Aug 4 2022, 10:44 PM

Add nounwind to remove cfi* directives.

LuoYuanke added inline comments.Aug 8 2022, 10:59 PM
llvm/test/CodeGen/X86/bfloat.ll
28

Parameter %c is not used, so we can delete it.

97

The %add is not converted to bfloat from float and it is converted to double directly. Is this the expected behaviour?

134

1.0 is in float format. Is it the expected behaviour?

pengfei updated this revision to Diff 451071.Aug 9 2022, 2:13 AM
pengfei marked an inline comment as done.

Address review comment.

llvm/test/CodeGen/X86/bfloat.ll
97

Good catch! I believe it is a bug, but it's not related to this patch. I'll do a follow up.

134

We don't use extra label to represent the FP constant in LLVM IR, the type bfloat has already indicated it. https://llvm.org/docs/LangRef.html#simple-constants

LuoYuanke accepted this revision.Aug 9 2022, 2:22 AM

LGTM.

This revision is now accepted and ready to land.Aug 9 2022, 2:22 AM
pengfei added inline comments.Aug 9 2022, 8:49 AM
llvm/test/CodeGen/X86/bfloat.ll
97

Did an experiment on D131502. But I don't want to rush it for 15.0 release given the problem has been existing on many targets using TypePromoteFloat, https://godbolt.org/z/cqooE1zT3

This revision was landed with ongoing or failed builds.Aug 9 2022, 6:41 PM
This revision was automatically updated to reflect the committed changes.

I think this broke some integration test using bf16 here? https://lab.llvm.org/buildbot/#/builders/61/builds/30600

Is this what we had already with https://github.com/llvm/llvm-project/issues/55992 ?

I think so. I have filed a new one: https://github.com/llvm/llvm-project/issues/57042

Please also do whatever is necessary to unbreak the bot.

Disabled the tests for now. Bot went green again: https://lab.llvm.org/buildbot/#/builders/61