[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:2320
+  case Intrinsic::is_fpclass: {
+    unsigned Flags = MachineInstr::copyFlagsFromInstruction(CI);
+
----------------
arsenm wrote:
> JanekvO wrote:
> > arsenm wrote:
> > > This should get an IRTranslator test to make sure the flags are passed through
> > Not sure if I completely hit the mark with my added test, but to me it seemed that not all flags were possible (e.g., `nnan` flag didn't work as it required a fp return type). For now I've added flag related tests that explicitly test the addition of `nofpexcept`. Do let me know if there's something missing or whether this `copyFlagsFromInstruction` is better omitted.
> I'd consider that a pre-existing bug in intrinsics. The IR is annoyingly strict about what things are allowed to have flags 
You can remove the flag copy if you want, although the flags may be introduced in the future 


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2333-2334
+    const Function *F = CI.getFunction();
+    if (!F->getAttributes().hasFnAttr(llvm::Attribute::StrictFP))
+      IsFpclass.setMIFlag(MachineInstr::NoFPExcept);
+
----------------
I just realized there's no point in doing this. G_IS_FPCLASS is not marked as mayRaiseFPException, so the flag is implied


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-fpclass-flags.ll:16
+}
+
+declare i1 @llvm.is.fpclass.f32(float, i32)
----------------
JanekvO wrote:
> arsenm wrote:
> > Needs additional checks with other flags besides the one just set
> I've been wondering whether the flag copy from the IR intrinsic to G_IS_FPCLASS in IRTranslator should be removed altogether. I'd have to weaken the flags' constraints as they all require scalar or vector fp return types. Additionally, Any use of fast math flags outside of existing uses will most likely require amending langref. E.g., current descriptions of some fast math flags describe how input can result into a poison value but this wouldn't be possible for G_IS_FPCLASS as it's a bool return.
> 
> Let me know what you think, I can see some of the flags being useful by folding into constant bool values (e.g., not a nan flag + G_IS_FPCLASS test for nans) but I may be a bit naïve on useful cases beyond said folding.
OK, might as well drop this test if we have end to end tests and there's nothing unique to test in the IRTranslator 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135447