This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add bf16 as a valid type for the FPR16 register class.
ClosedPublic

Authored by craig.topper on Jul 16 2023, 10:20 PM.

Details

Summary

This makes it possible for D153234 to use the FPR16 register class
for bf16 instructions.

isel patterns updated to have explicit f16 types due to type inference being disabled now.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 16 2023, 10:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2023, 10:20 PM
craig.topper requested review of this revision.Jul 16 2023, 10:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2023, 10:20 PM
Herald added subscribers: eopXD, MaskRay. · View Herald Transcript
craig.topper edited the summary of this revision. (Show Details)Jul 16 2023, 10:21 PM
joshua-arch1 added a comment.EditedJul 16 2023, 10:46 PM

Why are some patterns using FPR32 and FPR64 modified too?

Why are some patterns using FPR32 and FPR64 modified too?

Because they use the same classes in tablegen and I had to add a ValueType parameter to them.

Why are some patterns using FPR32 and FPR64 modified too?

Because they use the same classes in tablegen and I had to add a ValueType parameter to them.

I suppose that will be hard to maintain. Can we custom codegen classes for f16/bf16 in tablegen?

Why are some patterns using FPR32 and FPR64 modified too?

Because they use the same classes in tablegen and I had to add a ValueType parameter to them.

I suppose that will be hard to maintain. Can we custom codegen classes for f16/bf16 in tablegen?

Can you explain why it will be hard to maintain? The complexity of supporting Zfinx/Zdinx/Zhinx and all the special patterns for Zfhmin already seems way worse than passing an extra Valuetype to some classes.

Can you explain why it will be hard to maintain? The complexity of supporting Zfinx/Zdinx/Zhinx and all the special patterns for Zfhmin already seems way worse than passing an extra Valuetype to some classes.

I see. It seems that passing Valuetype to some codegen classes is the most effective way to address this issue if you want to use the same register class for different types. What we need to do now is to confirm that reusing FPR16 for bf16 is actually better than adding a new register class.

craig.topper added a comment.EditedJul 16 2023, 11:22 PM

Can you explain why it will be hard to maintain? The complexity of supporting Zfinx/Zdinx/Zhinx and all the special patterns for Zfhmin already seems way worse than passing an extra Valuetype to some classes.

I see. It seems that passing Valuetype to some codegen classes is the most effective way to address this issue if you want to use the same register class for different types. What we need to do now is to confirm that reusing FPR16 for bf16 is actually better than adding a new register class.

AArch64 also takes the approach of using a single register class for fp16 and bf16. Maybe there's some discussion around that decision we can find?

Can you explain why it will be hard to maintain? The complexity of supporting Zfinx/Zdinx/Zhinx and all the special patterns for Zfhmin already seems way worse than passing an extra Valuetype to some classes.

I see. It seems that passing Valuetype to some codegen classes is the most effective way to address this issue if you want to use the same register class for different types. What we need to do now is to confirm that reusing FPR16 for bf16 is actually better than adding a new register class.

AArch64 also takes the approach of using a single register class for fp16 and bf16. Maybe there's some discussion around that decision we can find?

LGTM.

wangpc accepted this revision.Jul 17 2023, 6:10 AM

LGTM.
Explict type helps to understand the patterns sometimes.

This revision is now accepted and ready to land.Jul 17 2023, 6:10 AM
asb accepted this revision.Jul 17 2023, 8:09 AM

LGTM.

This revision was landed with ongoing or failed builds.Jul 17 2023, 8:31 AM
This revision was automatically updated to reflect the committed changes.