This is an archive of the discontinued LLVM Phabricator instance.

[VP][RISCV] Add vp.is.fpclass and RISC-V support
ClosedPublic

Authored by liaolucy on Jun 14 2023, 11:45 PM.

Details

Summary

There is no vp.fpclass after FCLASS_VL(D151176), try to support vp.fpclass.

Diff Detail

Event Timeline

liaolucy created this revision.Jun 14 2023, 11:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2023, 11:45 PM
liaolucy requested review of this revision.Jun 14 2023, 11:45 PM
arsenm added inline comments.Jun 15 2023, 6:21 AM
llvm/include/llvm/IR/Intrinsics.td
2010

should be is_fpclass. No is implies returning an i32 for the class, like fpclassify

liaolucy updated this revision to Diff 531967.Jun 15 2023, 6:54 PM
liaolucy marked an inline comment as done.
  1. change name to: vp.is.fpclass
  2. update docs/LangRef.rst, add `llvm.vp.is.fpclass.*`' Intrinsics
craig.topper retitled this revision from [VP][RISCV] Add vp.fpclass and RISC-V support to [VP][RISCV] Add vp..is.fpclass and RISC-V support.Jun 15 2023, 11:13 PM

Need Verifier.cpp to make sure only legal bits are set. See existing code for llvm.is.fpclass.

llvm/include/llvm/IR/Intrinsics.td
2010

DefaultAttrsIntrinsics<[LLVMScalarOrSameVectorWidth<0, llvm_i1_ty>],

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7980

We don't copy fast math flags for llvm.is.fpclass. Why do we copy them here?

llvm/lib/IR/IntrinsicInst.cpp
626

I think there are too many types here? Only two any types exist in the definition. And if we use LLVMScalarOrSameVectorWidth<0, llvm_i1_ty> for the destination, we only need one.

craig.topper retitled this revision from [VP][RISCV] Add vp..is.fpclass and RISC-V support to [VP][RISCV] Add vp.is.fpclass and RISC-V support.Jun 16 2023, 12:05 AM
liaolucy updated this revision to Diff 532647.Jun 19 2023, 7:35 AM
liaolucy marked 2 inline comments as done.

updated

  1. Add Verifier.cpp to make sure only legal bits are set.
  2. Add more testcase
  3. address more comments

thanks

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7980

I tried to refer to the is_fpclass reserved flag before, but got it wrong. It looks normal now.
https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L6579

arsenm added inline comments.Jun 19 2023, 7:38 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7981

This should be unnecessary, by definition this is not a floating point operation and cannot raise FP exceptions

fakepaper56 added inline comments.Jun 19 2023, 8:50 AM
llvm/lib/IR/Verifier.cpp
6082

Is using auto for casting better?

craig.topper added inline comments.Jun 19 2023, 12:48 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7981

Doesn't this code also exist for Intrinsic::is_fpclass?

arsenm added inline comments.Jun 19 2023, 12:51 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7981

If it does it shouldn’t . Can either drop it or make it an unconditional true

liaolucy added inline comments.Jun 19 2023, 7:18 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7981

I tried to delete the flags in Intrinsic::is_fpclass and got the following diff for x86. Not sure if the diff is correct.

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index f38957637b74..bc8b1dddef71 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -6575,9 +6575,6 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
     MachineFunction &MF = DAG.getMachineFunction();
     const Function &F = MF.getFunction();
     SDValue Op = getValue(I.getArgOperand(0));
-    SDNodeFlags Flags;
-    Flags.setNoFPExcept(
-        !F.getAttributes().hasFnAttr(llvm::Attribute::StrictFP));
     // If ISD::IS_FPCLASS should be expanded, do it right now, because the
     // expansion can use illegal types. Making expansion early allows
     // legalizing these types prior to selection.
diff --git a/llvm/test/CodeGen/X86/is_fpclass.ll b/llvm/test/CodeGen/X86/is_fpclass.ll
index 6a531817e771..3b69011e3866 100644
--- a/llvm/test/CodeGen/X86/is_fpclass.ll
+++ b/llvm/test/CodeGen/X86/is_fpclass.ll
@@ -5,18 +5,18 @@
 define i1 @isnan_f(float %x) {
 ; CHECK-32-LABEL: isnan_f:
 ; CHECK-32:       # %bb.0: # %entry
-; CHECK-32-NEXT:    flds {{[0-9]+}}(%esp)
-; CHECK-32-NEXT:    fucomp %st(0)
-; CHECK-32-NEXT:    fnstsw %ax
-; CHECK-32-NEXT:    # kill: def $ah killed $ah killed $ax
-; CHECK-32-NEXT:    sahf
-; CHECK-32-NEXT:    setp %al
+; CHECK-32-NEXT:    movl $2147483647, %eax # imm = 0x7FFFFFFF
+; CHECK-32-NEXT:    andl {{[0-9]+}}(%esp), %eax
+; CHECK-32-NEXT:    cmpl $2139095041, %eax # imm = 0x7F800001
+; CHECK-32-NEXT:    setge %al
 ; CHECK-32-NEXT:    retl
 ;
 ; CHECK-64-LABEL: isnan_f:
 ; CHECK-64:       # %bb.0: # %entry
-; CHECK-64-NEXT:    ucomiss %xmm0, %xmm0
-; CHECK-64-NEXT:    setp %al
+; CHECK-64-NEXT:    movd %xmm0, %eax
+; CHECK-64-NEXT:    andl $2147483647, %eax # imm = 0x7FFFFFFF
+; CHECK-64-NEXT:    cmpl $2139095041, %eax # imm = 0x7F800001
+; CHECK-64-NEXT:    setge %al
 ; CHECK-64-NEXT:    retq
 entry:
   %0 = tail call i1 @llvm.is.fpclass.f32(float %x, i32 3)  ; "nan"
@@ -26,18 +26,18 @@ entry:
......
arsenm added inline comments.Jun 22 2023, 5:59 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7981

The expansion code should be considering strictfp on the parent function, not the node itself

liaolucy updated this revision to Diff 533611.Jun 22 2023, 8:03 AM
liaolucy marked 2 inline comments as done.

address comments, thanks

arsenm accepted this revision.Aug 18 2023, 6:19 AM
This revision is now accepted and ready to land.Aug 18 2023, 6:19 AM
liaolucy updated this revision to Diff 553387.Aug 24 2023, 11:59 PM

Rebase. Thanks for review.

This revision was landed with ongoing or failed builds.Aug 25 2023, 12:41 AM
This revision was automatically updated to reflect the committed changes.