This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support mask policy for RVV IR intrinsics.
ClosedPublic

Authored by khchen on Feb 20 2022, 7:39 PM.

Details

Summary

Add the IsValidMaskPolicy flag to indicate the operations
result would be effected by the mask policy. (ex. mask operations).

It means RISCVInsertVSETVLI should decide the mask policy according
by mask policy operand or passthru operand.
If IsValidMaskPolicy is false (ex. nomask, store, reduction operations.),
the mask policy could be either mask undisturbed or agnostic.
Currently, RISCVInsertVSETVLI sets ValidMaskPolicy operation default to
use MA, otherwise to MU to keep the current mask policy would not changed
for nomask operstions.

Add masked-tama, masked-tamu, masked-tuma and masked-tumu test cases.
I didn't add all operations because most of implementations are using
the same pseudo multiclass. Some test maybe be duplicated in different
tests. (ex. masked vmacc with tumu shows in vmacc-rv32.ll and masked-tumu.)
I think having different tests only for policy would make the test
clear.

Diff Detail

Event Timeline

khchen created this revision.Feb 20 2022, 7:39 PM
khchen requested review of this revision.Feb 20 2022, 7:39 PM
craig.topper added inline comments.Feb 20 2022, 9:08 PM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
516

We don't need a ternary operator here. IsValidMaskPolicy is a bool. You can do bool MaskAgnostic = IsValidMaskPolicy

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

VPseudoBinaryMask appears to only be used by VPseudoTernary which is used by reductions. But reduction shouldn't have IsValidMaskPolicy?

craig.topper added inline comments.Feb 20 2022, 9:09 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
96

I wonder if "UsesMaskPolicy" might be better?

khchen updated this revision to Diff 410239.Feb 21 2022, 12:42 AM

Address Craig's comments, thanks!

khchen added inline comments.Feb 21 2022, 12:44 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1153

Yes, I missed it again, thanks!

khchen updated this revision to Diff 410420.Feb 21 2022, 5:37 PM

update comment and refine code.

khchen marked 2 inline comments as done.Mar 16 2022, 8:57 PM

ping.

Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 8:57 PM
craig.topper added inline comments.Mar 21 2022, 7:23 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
93

nomask-> unmasked to keep with the terminology in the spec

Comma after "store"

119–120

Is 0 now TAIL_UNDISTURBED_MASK_UNDISTURBED? And 1 TAIL_AGNOSTIC_MASK_UNDISTURBED, etc. Or maybe we just remove TAIL_UNDISTURBED?

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
513

Is this a FIXME suggestion?

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

Is this used? These names are also now ambiguous and tablegen can't easily or them

khchen updated this revision to Diff 417172.Mar 21 2022, 8:25 PM
khchen marked 5 inline comments as done.

Address Craig's comments. Thanks!

craig.topper accepted this revision.Mar 21 2022, 9:01 PM

LGTM other than those last comments.

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
513

clang-format

522–531

use uint64_t so the assert won't allow negative numbers?

This revision is now accepted and ready to land.Mar 21 2022, 9:01 PM
This revision was landed with ongoing or failed builds.Mar 22 2022, 1:28 AM
This revision was automatically updated to reflect the committed changes.
khchen marked 2 inline comments as done.
arcbbb added inline comments.Apr 6 2022, 6:55 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
119–120

Sorry I didn't notice this change.
I saw there are two places using 0 as TAIL_UNDISTURBED.
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
It looks like policy is a 2-bit value representing TU_MU, TA_MU, TU_MA, and TA_MA as below

TU_MU0
TA_MU1
TU_MA2
TA_MA3

Maybe we still need the TU definition in enum.

khchen added inline comments.Apr 6 2022, 7:35 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
119–120

Do you mean in order to make code more consistent (enum in RISCVBaseInfo.h and TAIL_UNDISTURBED defined in td) we need to add TAIL_UNDISTURBED back to enum?
I think it's ok to me, but we should also update them as explicitly name like TU_MU or TAIL_UNDISTURBED_MASK_UNDISTURBED since we start model mask policy now?

arcbbb added inline comments.Apr 6 2022, 8:57 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
119–120

I got it. I thought 0 was undefined at my first glance on the enum.
I think it's fine to leave a comment here.

llvm/test/CodeGen/RISCV/rvv/masked-tumu.ll