[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 marked 2 inline comments as done.
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:
> > > 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.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2450
+    Observer.changingInstr(MI);
+    widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_FPEXT);
+    Observer.changedInstr(MI);
----------------
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?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6492-6493
     // legalizing these types prior to selection.
-    if (!TLI.isOperationLegalOrCustom(ISD::IS_FPCLASS, ArgVT)) {
+    if (!TLI.isOperationLegal(ISD::IS_FPCLASS, ArgVT) &&
+        !TLI.isOperationCustom(ISD::IS_FPCLASS, ArgVT)) {
       SDValue Result = TLI.expandIS_FPCLASS(DestVT, Op, Test, Flags, sdl, DAG);
----------------
foad wrote:
> Why did this change?
`isOperationLegalOrCustom` will return false if the type is considered illegal regardless of whether the instruction's type is marked legal or custom whereas `isOperationCustom` won't explicitly check for type legality and returns whether the action was set to custom. I basically just wanted it to go through to target custom code.

(May revert this in favor of using the expand code for f16 in case there is no f16 fp class instruction for amdgpu)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135447