[PATCH] D135447: [AMDGPU] Add llvm.is.fpclass intrinsic to existing SelectionDAG fp class support and introduce GlobalISel implementation for AMDGPU

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2332
+        .addImm(TestMaskValue.getZExtValue())
+        .addImm((unsigned)APFloat::SemanticsToEnum(FpSem))
+        .setMIFlag(MachineInstr::NoFPExcept);
----------------
sepavloff wrote:
> JanekvO wrote:
> > arsenm wrote:
> > > JanekvO wrote:
> > > > arsenm wrote:
> > > > > JanekvO wrote:
> > > > > > arsenm wrote:
> > > > > > > JanekvO wrote:
> > > > > > > > arsenm wrote:
> > > > > > > > > Do you really need the float type operand? I know bfloat16 isn't going to work without it, but I thought the plan was to introduce FP types to LLT
> > > > > > > > I believe it's not necessary for amdgpu but required for the `G_IS_FPCLASS` target opcode. Leaving it out results in verifier errors (I also am unaware about introducing FP types and LLT).
> > > > > > > What do you mean verifier errors?
> > > > > > Sorry, I meant that the MachineVerifier will fail for the `G_IS_FPCLASS` instruction.
> > > > > Right, it's there in the operand list. I mean more abstractly, why is it there?
> > > > I can see the semantics being used in the target independent expansion of `IS_FPCLASS` in SelectionDAG (e.g., for retrieving `inf` of a particular fp semantic). I'm inferring that the rationale could be: GlobalISel will require a similar implementation and therefore requires the semantics. I haven't looked into whether any alternatives exist that don't require passing of the semantics through the operand, though.
> > > Currently the LLT directly implies the semantics for every operation
> > @sepavloff Do you happen to recall the rationale of the fp semantic operand for `G_IS_FPCLASS`? My knowledge about it are a bit shallow but perhaps it can be removed
> It is used to workaround limitations of GlobalISel, - lack of floating-point types. Without this operand it is impossible to distinguish between `half` and `bfloat16` and also between different flavors of 8-bit floats.
> 
> If LLT supported floating-point types, this operand could be removed.
But this is a problem for every single operation, not just this one. We don't have a decided upon strategy for dealing with this, so it doesn't make sense to me to try to deal with it here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135447