[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);
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:318-320
+      {MVT::v2f16, MVT::v3f16, MVT::v4f16, MVT::v16f16, MVT::v2f32, MVT::v3f32,
+       MVT::v4f32, MVT::v5f32, MVT::v6f32, MVT::v7f32, MVT::v8f32, MVT::v16f32,
+       MVT::v2f64, MVT::v3f64, MVT::v4f64, MVT::v8f64, MVT::v16f64},
----------------
JanekvO wrote:
> arsenm wrote:
> > foad wrote:
> > > It seems annoying to have such a long list of types here - it'll need updating whenever we introduce a new one. Can you use something like FloatVectorTypes instead?
> > This should be unnecessary, we have no vector class instructions. These should just expand into scalars
> If not set as custom (or legal), these'll get expanded through the target independent expansion. Bypassing said target independent expansion does result into the desired scalarizing.
This is one of the problems with doing this kind of expansion in SelectionDAGBuilder. This should go through the usual legalization paths 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135447