Pierre-vh added inline comments.
================ Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:5573-5576 + SDLoc SL(Op); + return DAG.getNode( + ISD::FP_EXTEND, SL, MVT::f32, + DAG.getNode(ISD::BITCAST, SL, MVT::f16, Op->getOperand(0))); ---------------- arsenm wrote: > arsenm wrote: > > Pierre-vh wrote: > > > arsenm wrote: > > > > ExpandNode covers lowering BF16_TO_FP. It also has a shift by 16-bits > > > > into the high bits. Is this correct? > > > Ah I didn't know that, though as long as we use custom lowering, and our > > > FP_TO_BF16/BF16_TO_FP methods are consistent, it should be fine, no? > > bfloat16 has the same number of exponent bits in the same high bits as f32; > > I kind of think the idea is you can just do a bitshift and then operate on > > f32? I think the fp_extend here is wrong > The default legalization also looks wrong to me. I don't understand why it > isn't shifting down the mantissa bit Indeed it was terribly wrong. I rewrote both legalizations following what I found online: https://en.wikipedia.org/wiki/Bfloat16_floating-point_format bf16 is designed to be very easily convertible from/to f32, save for some edge cases with denormalized numbers I think, thus: - bf16 -> f32 is just left-shift by 16, filling the least-significant bits with zeroes. - f32 -> bf16 is just cutting off the 16 least-significant bits. ================ Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4819-4831 + // When we don't have 16 bit instructions, bf16 is illegal and gets + // softened to i16 for storage, with float being used for arithmetic. + // + // After softening, some i16 -> fp32 bf16_to_fp operations can be left over. + // Lower those to (f32 (fp_extend (f16 (bitconvert x)))) + if (!Op->getValueType(0).isFloatingPoint() || + Op->getOperand(0).getValueType() != MVT::i16) ---------------- arsenm wrote: > Pierre-vh wrote: > > arsenm wrote: > > > Pierre-vh wrote: > > > > arsenm wrote: > > > > > Pierre-vh wrote: > > > > > > arsenm wrote: > > > > > > > Pierre-vh wrote: > > > > > > > > arsenm wrote: > > > > > > > > > The generic legalizer should have handled this? > > > > > > > > It looks like those operations are not implemented in the > > > > > > > > generic legalizer, e.g. I get > > > > > > > > ``` > > > > > > > > Do not know how to promote this operator's operand! > > > > > > > > ``` > > > > > > > Right, this is the code that would go there > > > > > > Do I just copy/paste this code in that PromoteInt function, and > > > > > > keep a copy here too in LowerOperation? (not really a fan of > > > > > > copy-pasting code in different files, I'd rather keep it all here) > > > > > > We need to have the lowering too AFAIK, it didn't go well when I > > > > > > tried to remove it > > > > > I'm not following why you need to handle it here > > > > IIRC: > > > > - I need to handle FP_TO_BF16 in ReplaceNodeResult because that's what > > > > the Integer Legalizer calls (through CustomLowerNode) > > > > - I need to handle both opcodes in LowerOperation because otherwise > > > > they'll fail selection. They can be left over from expanding/legalizing > > > > other operations. > > > But why are they custom? We don't have to handle FP16_TO_FP or FP_TO_FP16 > > > there, and they aren't custom lowered. They have the same basic > > > properties. We have this: > > > > > > > > > ``` > > > setOperationAction(ISD::FP16_TO_FP, MVT::i16, Promote); > > > AddPromotedToType(ISD::FP16_TO_FP, MVT::i16, MVT::i32); > > > setOperationAction(ISD::FP_TO_FP16, MVT::i16, Promote); > > > AddPromotedToType(ISD::FP_TO_FP16, MVT::i16, MVT::i32); > > > ``` > > > > > > I'd expect the same basic pattern > > PromoteIntegerOperand, PromoteFloatOperand and PromoteIntegerResult don't > > handle FP_TO_BF16 and BF16_TO_FP, and unless we put a Custom lowering mode > > it'll assert/unreachable. > > I tried to make it work (for a while) using the default expand but I can't > > quite get it to work. It feels like there is some legalizer work missing > > for handling BF16 like we want to. > > Even though it's not ideal I think the custom lowering is easiest > What about Expand? that's where the implemented part is Last I tried, Expand will emit a libcall in many cases that we don't handle Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139398/new/ https://reviews.llvm.org/D139398 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
