[PATCH] D139398: [AMDGPU] Add bf16 storage support

[Pierre van Houtryve via Phabricator via cfe-commits]

Pierre-vh added inline comments.


================
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:
> > > > > > > > > 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
> Library call is supposed to be a distinct action now, the DAG only did about 5% of the work to migrate to using it. This code can go to the default expand action
Does it need to happen in this commit? It'll delay the review quite a bit I think if other people have to review it
If it needs to happen, when what do I need to do? Use the Expand action & fix the legalizer in places where it needs to be fixed?

I feel like it might be better suited for a follow-up patch; I can create a task and pick it up when I come back from vacation if you want


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139398/new/

https://reviews.llvm.org/D139398