This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Remove experimental for zihintntl
ClosedPublic

Authored by jacquesguan on May 26 2023, 2:24 AM.

Details

Summary

Since zihintntl is ratified now, we could remove the experimental prefix and change its version to 1.0.

Diff Detail

Event Timeline

jacquesguan created this revision.May 26 2023, 2:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 2:24 AM
jacquesguan requested review of this revision.May 26 2023, 2:24 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 26 2023, 2:24 AM
asb added a comment.EditedMay 26 2023, 2:35 AM

Thanks for the patch.

Is the __riscv_ntl_* interface finalised and agreed? I'm wary of repeating the mistake we made with the V extension where we exposed intrinsics that weren't yet finalised.

Thanks for the patch.

Is the __riscv_ntl_* interface finalised and agreed? I'm wary of repeating the mistake we made with the V extension where we exposed intrinsics that weren't yet finalised.

I don't find any information about these two intrinsics, where do they be proposed?

asb added a comment.Jun 6 2023, 12:51 AM

@jacquesguan I'm not sure on the standardisation process or status for these intrinsics. Perhaps @kito-cheng has an idea?

BeMg added inline comments.Jul 19 2023, 1:04 AM
llvm/docs/RISCVUsage.rst
207–208

zihintntl could add into table:: Ratified Extensions by Status, and be removed this from the Experimental Extensions section.

BeMg added inline comments.Jul 19 2023, 1:15 AM
clang/test/Preprocessor/riscv-target-features.c
150–152

The CHECK-ZIHINTNTL-EXT also need update.

Address comment and rebase.

jacquesguan marked 2 inline comments as done.Jul 19 2023, 2:14 AM
jacquesguan added inline comments.
clang/test/Preprocessor/riscv-target-features.c
150–152

Done, thanks.

llvm/docs/RISCVUsage.rst
207–208

Done, thanks.

BeMg added inline comments.Jul 19 2023, 4:49 AM
llvm/unittests/Support/RISCVISAInfoTest.cpp
376 ↗(On Diff #541904)

These unit tests are used to test the experimental extension name parsing, it is not suitable to use zihintntl here because zihintntl is no longer experimental.

I update it by https://reviews.llvm.org/D155673. After rebasing, I think the test fail in RISCVISAInfoTest.cpp can be resolved.

asb added a comment.Jul 19 2023, 5:53 AM

I remain concerned about exposing the intrinsics if they're not yet agreed as finalised. I see there is now a PR to add them to riscv-c-api doc https://github.com/riscv-non-isa/riscv-c-api-doc/pull/47

I'd be OK with merging this now if the intrinsics were temporarily removed, or logic were added to gate them in some way (though we don't have precedent on the best way to do this I don't think). Otherwise, I'd rather wait until the intrinsics are agreed.

jacquesguan marked 2 inline comments as done.

rebase main and address comment.

I remain concerned about exposing the intrinsics if they're not yet agreed as finalised. I see there is now a PR to add them to riscv-c-api doc https://github.com/riscv-non-isa/riscv-c-api-doc/pull/47

I'd be OK with merging this now if the intrinsics were temporarily removed, or logic were added to gate them in some way (though we don't have precedent on the best way to do this I don't think). Otherwise, I'd rather wait until the intrinsics are agreed.

I saw that https://github.com/riscv-non-isa/riscv-c-api-doc/pull/47 is basically agreed and close to merge, I think we could wait it to avoid repeat patch.

asb added a comment.Jul 20 2023, 3:32 AM

@jacquesguan yes I agree, let's hold off on merging this until that PR is merged as it looks like it's quite close.

https://github.com/riscv-non-isa/riscv-c-api-doc/pull/47 is already merged, any more advice about this patch?

asb added a comment.Jul 25 2023, 1:23 AM

https://github.com/riscv-non-isa/riscv-c-api-doc/pull/47 is already merged, any more advice about this patch?

Just that it needs a release note, and we need the newly defined overloaded form of the intrinsics to be supported.

asb added a comment.Aug 9 2023, 3:38 AM

@jacquesguan: I think this is good to go if you rebase now D156221 landed and add a release note.

rebase and add release note.

jacquesguan retitled this revision from [RISCV] Remove experimental for zihintntl. to [RISCV] Remove experimental for zihintntl.Aug 9 2023, 6:55 PM
asb accepted this revision.Aug 9 2023, 10:17 PM

LGTM - just see the very minor corrections inline. Thanks!

clang/test/Preprocessor/riscv-target-features.c
146

Drop -menable-experimental-extensions

149

Drop Drop -menable-experimental-extensions`

llvm/docs/RISCVUsage.rst
123

zihintntl -> Zihintntl

This revision is now accepted and ready to land.Aug 9 2023, 10:17 PM

Address comment.

This revision was landed with ongoing or failed builds.Aug 10 2023, 2:05 AM
This revision was automatically updated to reflect the committed changes.