This is an archive of the discontinued LLVM Phabricator instance.

[1/15][Clang][RISCV][NFC] Extract common utility to RISCVVIntrinsicUtils
ClosedPublic

Authored by eopXD on Jan 12 2023, 12:49 AM.

Details

Summary

This is the 1st commit of a patch-set that aims to change the default policy
for RVV intrinsics from TAMU to TAMA.

The patch-set work towards the simplification proposal [0] of Nick
Knight. After this patch-set, all intrinsics operates under a general
assumption that the policy behavior is agnostic.

You may find that most of the patches are NFC patches. They subtly remove
implicit assumptions that entangles the codebase, making the singular patch
that contains functional change clear and obvious.

In [2/15], The attribute Policy::IsPolicyNone may give the mis-perception
that an RVV intrinsic may operate without any policy. However this is not the
case because the policy CSR-s (vta and vma) always affect an RVV
instruction's behavior, except that some instructions have policy always set
as agnostic (e.g. instructions with a mask destination register is always
tail agnostic).

Next, to perform the change from TAMU to TAMA, we need to first remove
Policy::PolicyType::Omit. [4/15] ~ [12/15] removes it with NFC patches step
by step. Without the patches, directly applying [14/15] to the existing codebase
will not work because there will be complicated logics that are scattered in
places that is hard to maintain.

[1/15], [3/15] are not related to the main goal of this patch-set, they were
clean-up along the way as I was going through the codebase. [13/15] is a
clean-up that was an oversight in D141198.

Finally, [14/15] performs the functional change this patch-set aims for. The
default policy is changed from TAMU to TAMA. This affects the masked version of
the intrinsics without a policy suffix. The masked-off operand is removed. Due
to the removal, masked version of vid and viota intrinsics are no longer
available for overloading.

[15/15] is a final commit to set data members of Policy as constants. Through
the refactoring the class Policy is now correct-by-construction.

The next patch-set will be to remove redundant intrinsics with a policy suffix
_ta and _tama intrinsics are redundant and will be removed. Other policy
suffix will be renamed to adapt to the general assumption that policy is
generally agnostic.

[0] https://gist.github.com/nick-knight/6cb0b74b351a25323dfb1821d3a269b9

Pull Request: riscv-non-isa/rvv-intrinsic-doc#186

Diff Detail

Event Timeline

eopXD created this revision.Jan 12 2023, 12:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 12:49 AM
eopXD requested review of this revision.Jan 12 2023, 12:49 AM
eopXD updated this revision to Diff 488578.Jan 12 2023, 3:13 AM

Bump CI

eopXD updated this revision to Diff 488822.Jan 12 2023, 5:50 PM

Bump CI

eopXD updated this revision to Diff 489285.Jan 14 2023, 10:55 AM

Rebase to latest main.

eopXD retitled this revision from [WIP][1/N][Clang][RISCV][NFC] Extract common utility to RISCVVIntrinsicUtils to [1/N][Clang][RISCV][NFC] Extract common utility to RISCVVIntrinsicUtils.Jan 15 2023, 7:19 AM
eopXD edited the summary of this revision. (Show Details)
eopXD retitled this revision from [1/N][Clang][RISCV][NFC] Extract common utility to RISCVVIntrinsicUtils to [1/15][Clang][RISCV][NFC] Extract common utility to RISCVVIntrinsicUtils.
eopXD edited the summary of this revision. (Show Details)Jan 15 2023, 9:59 AM
eopXD edited the summary of this revision. (Show Details)Jan 15 2023, 11:40 AM
This revision is now accepted and ready to land.Jan 15 2023, 5:55 PM
craig.topper accepted this revision.Jan 17 2023, 4:14 PM

LGTM

clang/lib/Support/RISCVVIntrinsicUtils.cpp
977

Possible improvement for a different patch:

Could these policy lists be static const arrays and this function could returned an ArrayRef pointing to the appropriate list instead constructing a SmallVector?

eopXD added inline comments.Jan 18 2023, 2:46 AM
clang/lib/Support/RISCVVIntrinsicUtils.cpp
977

Yes. I will use a flyweight pattern to simplify things here.

This revision was landed with ongoing or failed builds.Jan 24 2023, 12:01 AM
This revision was automatically updated to reflect the committed changes.