This is an archive of the discontinued LLVM Phabricator instance.

[Target][RISCV] Update SubtargetFeature definition for RV32/RV64 (NFCI).
ClosedPublic

Authored by fpetrogalli on Mar 15 2023, 4:47 AM.

Details

Summary

This is done for consistency with other Predicate/SubtargetFeature pairs, where the second parameter of the SubtargetFeature correspond to the NAME of the def of the Predicate associated to the SubtargetFeature.

Diff Detail

Event Timeline

fpetrogalli created this revision.Mar 15 2023, 4:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2023, 4:47 AM
fpetrogalli requested review of this revision.Mar 15 2023, 4:47 AM
fpetrogalli edited the summary of this revision. (Show Details)Mar 15 2023, 4:48 AM
asb added a comment.Mar 15 2023, 4:58 AM

Thanks for making this suggestion.

The suggested renaming would be consistent with those other predicates, but RV{32,64}{I,E} are different base ISAs while the other predicates refer to instruction set extensions. Because of this logical distinction IsRV32/IsRV64 make more sense to me at least.

It's a bit less clear for RVE given the way me model it within LLVM is somewhat like as if it was an instruction set extension. Though I still think I personally prefer IsRVE for that case.

Thanks for making this suggestion.

The suggested renaming would be consistent with those other predicates, but RV{32,64}{I,E} are different base ISAs while the other predicates refer to instruction set extensions. Because of this logical distinction IsRV32/IsRV64 make more sense to me at least.

It's a bit less clear for RVE given the way me model it within LLVM is somewhat like as if it was an instruction set extension. Though I still think I personally prefer IsRVE for that case.

Oh I see - thank you for explaining.

Would it be OK if I change the SubtargetFeature param from "HasRV32" to "IsRV32" then? (same for the 64 bit one) So that we can associate SubtargetFeature and Predicate.

I have updated the patch modifying the SubtargetFeature definition instead of the Predicate NAME.

fpetrogalli retitled this revision from [Target][RISCV] Fix inconsistent naming of Predicates (NFCI). to [Target][RISCV] Update SubtargetFeature definition for RV32/RV64 (NFCI)..Mar 15 2023, 7:14 AM
fpetrogalli edited the summary of this revision. (Show Details)
asb accepted this revision.Mar 15 2023, 7:18 AM

LGTM - thanks!

This revision is now accepted and ready to land.Mar 15 2023, 7:18 AM