Support for bfloat in DAGTypeLegalizer::SoftenFloatRes_FP_EXTEND

Hi folks,

I’m new to LLVM discourse, so thanks in advance for your patience and understanding :).

I’m working with a proprietary target that has native bfloat, but no native double precision. DAGTypeLegalizer::SoftenFloatRes_FP_EXTEND doesn’t check for bf16, so lowering of FP_EXTEND bf16 → f64 asserts with “Unsupported FP_EXTEND!”. This is easily fixed with the change below, however I would like to make this change upstream, if practical, rather than maintain another patch in our internal LLVM fork. Is there an existing upstream target for which this scenario is relevant, and therefore could be used to write a test case against? If not, any suggestions for how I might create a meaningful test case in the absence of a relevant target?

Many thanks in advance,

Henry

--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
@@ -513,7 +513,8 @@ SDValue DAGTypeLegalizer::SoftenFloatRes_FP_EXTEND(SDNode *N) {
   // There's only a libcall for f16 -> f32, so proceed in two stages. Also, it's
   // entirely possible for both f16 and f32 to be legal, so use the fully
   // hard-float FP_EXTEND rather than FP16_TO_FP.
-  if (Op.getValueType() == MVT::f16 && N->getValueType(0) != MVT::f32) {
+  if ((Op.getValueType() == MVT::f16 || Op.getValueType() == MVT::bf16) &&
+      N->getValueType(0) != MVT::f32) {
     if (IsStrict) {
       Op = DAG.getNode(ISD::STRICT_FP_EXTEND, SDLoc(N),
                        { MVT::f32, MVT::Other }, { Chain, Op });

We sometimes accept small, obvious, patches to codegen without tests if there’s no reasonable way to test them with an in-tree target.

Alternatively, you could make your target custom legalize FP_EXTEND.

No. bfloat has limited support only on ARM/AArch64/X86 in upstream ATM. None of them has native support.

I agreed with @efriedma-quic , custom is a better choice though it’s also feasible to change without test.

Make sure the comments are changed together if you want to do the patch and note there’s no bf16 → f32 libcall either. We use 16 bit shift instead, see D126953.

Thanks for the replies, @efriedma-quic and @phoebe. Regarding custom legalization of FP_EXTEND, I don’t see an obvious target hook in DAGTypeLegalizer. There’s a pre-type-legalization DAG combine, but on face value, performing legalization during an optimization pass strikes me as kludgy. Am I overthinking that? :slight_smile: Is there a more appropriate target hook before or during type legalization that I’ve overlooked?

Thanks again,

Henry

If you setOperationAction(ISD::FP_EXTEND, MVT::f64, Custom); or something like that, the type legalizer will call your target’s implementation of ReplaceNodeResults.

Thanks. My read of the code was that LLVM only calls CustomLowerNode when legalizing via expansion or promotion. DAGTypeLegalizer::SoftenFloatResult explicitly doesn’t call it (but perhaps it should?). I tried your suggestion anyway, in case I read it wrong, but didn’t hit a breakpoint in ReplaceNodeResults prior to the assertion in SoftenFloatRes_FP_EXTEND.

You’re right. Didn’t realize that was a thing; almost all other forms of legalization call CustomLowerNode. Probably should be fixed.

I added calls to CustomLowerNode in the ‘soften’ paths, and not entirely surprisingly, several in-tree-target tests failed in ReplaceNodeResults due to unhandled ops. It strikes me that modifying ReplaceNodeResults in the relevant targets to explicitly handle all of their respective ops would come with a significant regression risk. Thinking out loud, we could instead add an ‘enableCustomLowerSoftenFloat’ virtual function call to TargetLowering, which returns ‘false’ by default, and then only call CustomLowerNode in the ‘soften’ paths if that returns ‘true’. Or we could change the signatures of CustomLowerNode, ReplaceNodeResults, etc. to pass through the LegalizeTypeAction and then have existing in-tree targets early exit when the value is TypeSoftenFloat.

Any thoughts? Thanks in advance for your insight.

How about make it legal type and do promotion for all other operations? Like the way I did for FP16. Reland "Reland "Reland "Reland "[X86][RFC] Enable `_Float16` type sup… · llvm/llvm-project@655ba9c · GitHub
Then we are able to custom FP_EXTEND.