This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][RVV] Add strict vfcvt intrinsics that have side effects for dynamically-set rounding mode
Changes PlannedPublic

Authored by arcbbb on Feb 23 2022, 7:11 PM.

Details

Summary

This patch adds a set of vector vfcvt intrinsics that have side effects:

strict.vfcvt.xu.f,  strict.vfcvt.x.f,  strict.vfcvt.f.xu, strict.vfcvt.f.xu,
strict.vfwcvt.xu.f,  strict.vfwcvt.x.f,  strict.vfwcvt.f.xu, strict.vfwcvt.f.xu, strict.vfwcvt.f.f
strict.vfncvt.xu.f,  strict.vfncvt.x.f,  strict.vfncvt.f.xu, strict.vfncvt.f.xu,.strict.vfncvt.f.f

These strict intrinsics are intended to work with fesetround() call when FENV_ACCESS is on.

And the rest strict vector fp intrinsics will come in later patches:

vfsqrt, vfrsqrt7, vfrec7, vfadd, vfsub, vfmul, vfdiv,
vfredosum, vfredusum, vfmacc, vfmadd, vfmsac, vfmsub

Diff Detail

Event Timeline

arcbbb created this revision.Feb 23 2022, 7:11 PM
arcbbb requested review of this revision.Feb 23 2022, 7:11 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 23 2022, 7:11 PM
arcbbb updated this revision to Diff 412356.Mar 2 2022, 1:51 AM
arcbbb edited the summary of this revision. (Show Details)

Update:

  1. Remove Clang changes to make this patch smaller for review
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 1:51 AM
kito-cheng added inline comments.Mar 2 2022, 3:34 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4683

Is this operand for tail policy? if so why this is UNDEF? I guess this should be TAIL_AGNOSTIC rather than UNDEF?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
4673

I saw all other target using 0/1 rather than true/false including RISCVInstrInfoF.td/RISCVInstrInfoD.tdwhen setting mayRaiseFPException, maybe we should keep it consistent?

4673

And I think we should also set mayRaiseFPException in RISCVInstrInfoV.td.

craig.topper added inline comments.Mar 2 2022, 3:16 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4671

Drop curly braces

4675

Drop curly braces

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
4673

true/false are relatively new to tablegen. They were introduced in Nov. 2020. I don't think I've fully adjusted to their existence so I probably put 0/1 in the scalar code out of habit.

arcbbb updated this revision to Diff 412637.Mar 3 2022, 2:01 AM

Updates:

  1. Adds Uses and mayRaiseFPException to VFCVT* Instruction in RISCVInstrInfoV.td
  2. Drops curly braces in function lowerRVVStrictIntrinsics
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4683

Because unmasked pseudos doesn't have a policy operand,
TA/TU is distinguished by checking if passthru is undef. I think it is proper to leave undef here.

khchen added a comment.Mar 4 2022, 8:01 AM

Makes sense to me, but I'd appreciate someone else for a final LGTM.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4678

I think TrueMask maybe better.

4683

nit: maybe we could add a comment for that, because it must be UNDEF to match unmaksed pattern successfully.

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
619

--> VReg op2_reg_class> {

649

This is additional newline comparing to VPatConvertI2FPSDNode_V_VL_STRICT? or we need a newline in VPatConvertI2FPSDNode_V_VL_STRICT?

craig.topper added inline comments.Mar 4 2022, 9:54 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
624

I don't like using Undef for a field that should be a constant. Can we use 0?

arcbbb updated this revision to Diff 413332.Mar 6 2022, 8:18 PM

Updates:

  1. Fixes indentation in RISCVInstrInfoVVLPatterns.td
  2. Changes variable name Mask to TrueMask in lowerRVVStrictIntrinsics().
  3. Use immediate value 0 in the policy operand for unmasked pseudos.
arcbbb updated this revision to Diff 415912.Mar 16 2022, 11:30 AM

rebase to main

arcbbb planned changes to this revision.Mar 23 2022, 9:45 PM

I started a discussion on how RVV clang builtins interact with FENV_ACCESS in https://github.com/riscv-non-isa/rvv-intrinsic-doc/issues/147
I would drop this patch if those builtins are going to be independent of FENV.