Since zihintntl is ratified now, we could remove the experimental prefix and change its version to 1.0.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
@jacquesguan I'm not sure on the standardisation process or status for these intrinsics. Perhaps @kito-cheng has an idea?
llvm/docs/RISCVUsage.rst | ||
---|---|---|
207–208 | zihintntl could add into table:: Ratified Extensions by Status, and be removed this from the Experimental Extensions section. |
clang/test/Preprocessor/riscv-target-features.c | ||
---|---|---|
146–152 | The CHECK-ZIHINTNTL-EXT also need update. |
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. |
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.
@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?
Just that it needs a release note, and we need the newly defined overloaded form of the intrinsics to be supported.
@jacquesguan: I think this is good to go if you rebase now D156221 landed and add a release note.
Drop -menable-experimental-extensions