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

[Janek van Oirschot via Phabricator via llvm-commits]

JanekvO added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2332
+        .addImm(TestMaskValue.getZExtValue())
+        .addImm((unsigned)APFloat::SemanticsToEnum(FpSem))
+        .setMIFlag(MachineInstr::NoFPExcept);
----------------
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.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2450
+    Observer.changingInstr(MI);
+    widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_FPEXT);
+    Observer.changedInstr(MI);
----------------
arsenm wrote:
> JanekvO wrote:
> > arsenm wrote:
> > > arsenm wrote:
> > > > This will do the wrong thing for snans and also denormals inputs are flushed 
> > > I also don't see the corresponding DAG legalization. It's such a special case I think this should be split into a separate patch anyway.
> > > I also don't see the corresponding DAG legalization.
> > I put the corresponding SelectionDAG type widening code for `IS_FPCLASS` is in target custom function `LowerIS_FPCLASS` as I couldn't bypass expansion in SelectionDAGBuilder.cpp when marking the action for the instruction with f16 as `promote` (i.e., it would call `IS_FPCLASS` expansion code even when trying to promote).
> > 
> > > It's such a special case I think this should be split into a separate patch anyway.
> > As in, the widening code, or `IS_FPCLASS` support for amdgpu gfx7?
> OK, there are several issues here. None of this should be done in target code.
> 
> I also don't approve of doing this expansion in the DAG builder, but see that's a pre-existing issue. GlobalISel does need to do the same expansion. 
I was looking at implementing the SelectionDAG target independent expansion for GlobalISel `lower()`. I'll first remove f16 legalizing for cases where there is no f16 instructions available for amdgpu for this diff and move the GlobalISel's expansion/lower to another diff.


================
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},
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135447