[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. kito-cheng marked an inline comment as done. Closed by commit rG7a5cb15ea6fa: [RISCV]

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng marked 9 inline comments as done. kito-cheng added a comment. @aaron.ballman thanks for your review! Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:400 + // Number of fields, greater than 1 if it's segment load/store. + uint8_t NF; +};

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM, only minor nits found (feel free to ignore any that you disagree with). Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:346 +// RVVRequire should be sync with target features, but only

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added a comment. @aaron.ballman do you mind give few more look on this patch, we would like gather LGTM from both RISC-V folks and clang folks, thanks :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 444188. kito-cheng added a comment. Changes: - Correct filename for testcases. - Use forward declaration for llvm::raw_ostream Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

craig.topper added inline comments. Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:17 #include "llvm/ADT/StringRef.h" +#include "llvm/Support/raw_ostream.h" #include Do we need the header or is a forward declaration enough? Repository: rG

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

craig.topper added inline comments. Comment at: clang/test/Sema/riscv-bad-intrnisic-pragma.c:1 +// RUN: %clang_cc1 -triple riscv64 -target-feature +v %s -emit-llvm -o - \ +// RUN:2>&1 | FileCheck %s this test file name is misspelled

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 442431. kito-cheng added a comment. Changes: Restore the patch, I just accidentally updated wrong revision here... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 442429. kito-cheng added a comment. Changes: - Less invasive way to fix this issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 Files:

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added inline comments. Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:100 +switch (Type->getElementBitwidth()) { +case 64: + QT = Context.DoubleTy; aaron.ballman wrote: > kito-cheng wrote: > > aaron.ballman wrote: > > > I almost

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 442025. kito-cheng marked 9 inline comments as done. kito-cheng added a comment. Changes: - Address @frasercrmck's comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

aaron.ballman added inline comments. Comment at: clang/lib/Parse/ParsePragma.cpp:3963 +<< PP.getSpelling(Tok) << "riscv" << /*Expected=*/true << "'intrinsic'"; +return; + } kito-cheng wrote: > aaron.ballman wrote: > > It's fine to warn on this, but

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added inline comments. Comment at: clang/lib/Parse/ParsePragma.cpp:3963 +<< PP.getSpelling(Tok) << "riscv" << /*Expected=*/true << "'intrinsic'"; +return; + } aaron.ballman wrote: > It's fine to warn on this, but then you need to eat

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 441692. kito-cheng marked 18 inline comments as done. kito-cheng added a comment. Changes: - Address @aaron.ballman’s comment - Add 2 new testcase: - riscv-bad-intrnisic-pragma.c - riscv-intrnisic-pragma.c Repository: rG LLVM Github Monorepo

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

frasercrmck added a comment. Just nits from me. Comment at: clang/include/clang/Sema/RISCVIntrinsicManager.h:9 +// +// This file defines the RISCVIntrinsicManager, which handle RISC-V vector +// intrinsic functions. `handles` Comment at:

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 441684. kito-cheng added a comment. Changes: - Address @craig.topper's comment - Introduce RISCVIntrinsicManager.h and let it become member of Sema, that make sure the it won't outlive than Sema. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

aaron.ballman added a comment. Some drive-by comments from the peanut gallery. Comment at: clang/lib/Parse/ParsePragma.cpp:3963 +<< PP.getSpelling(Tok) << "riscv" << /*Expected=*/true << "'intrinsic'"; +return; + } It's fine to warn on this, but

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng marked an inline comment as done. kito-cheng added a comment. Oh, have one comment not address yet Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added inline comments. Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:114 bool operator>(const PrototypeDescriptor ) const { -return !(PD.PT <= PT && PD.VTM <= VTM && PD.TM <= TM); +if (PD.PT != PT) + return PD.PT > PT;

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 441610. kito-cheng marked 24 inline comments as done. kito-cheng added a comment. Changes: - Address @craig.topper's comment - Address @khchen's comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

craig.topper added inline comments. Comment at: clang/include/clang/Basic/TokenKinds.def:911 +// Annotation for the riscv pragma directives - #pragma clang riscv intrinsic .. +PRAGMA_ANNOTATION(pragma_riscv) Why only 2 periods at the end. Should be 3 like on

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

khchen added a comment. @craig.topper @rogfer01 - do you have other comments? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 ___ cfe-commits mailing list

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

khchen accepted this revision. khchen added a comment. This revision is now accepted and ready to land. LGTM. Other than that last comments. Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:77 + // Create compressed hsignature table from SemaRecords. + void init(const

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 441320. kito-cheng added a comment. Changes: - Rebase - Address @khchen's comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 Files:

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

khchen added inline comments. Comment at: clang/lib/Sema/SemaRVVLookup.cpp:175 + for (auto : RVVIntrinsicRecords) { +// Create Intrinsics for each type and LMUL. +BasicType BaseType = BasicType::Unknown; Those code logic need to sync with

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added inline comments. Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:480 +// They are handled by riscv_vector.h +if (Name == "vsetvli" || Name == "vsetvlimax") + continue; khchen wrote: > I feel little tricky to checking the name

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 431646. kito-cheng marked 5 inline comments as done. kito-cheng added a comment. Changes: - Split out several NFC changes to individual NFC patchs. - Moving most code emission logic into RISCVVIntrinsicUtils to prevent require sync manually. - PCH

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

khchen added a comment. Do we need to have some tests in `clang/test/PCH/` for new #pragma? Comment at: clang/lib/Sema/SemaLookup.cpp:932 + if (DeclareRVVBuiltins) { +if (GetRVVBuiltinInfo(*this, R, II, PP)) { + return true; Don’t Use

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added inline comments. Comment at: clang/lib/Sema/SemaRVVLookup.cpp:91 +struct RVVIntrinsicDef { + std::string Name; + std::string GenericName; khchen wrote: > why do we need to declare Name as std::string here but RVVIntrinsicRecord use > `const

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 426285. kito-cheng marked 2 inline comments as done. kito-cheng added a comment. Changes: - Minor tweak. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 Files:

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 426284. kito-cheng marked 4 inline comments as done. kito-cheng added a comment. Changes: - Add more comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 426281. kito-cheng marked 2 inline comments as done. kito-cheng added a comment. Changes: - Split out refactor part to D124730 . - Add more comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

khchen added a comment. Herald added a subscriber: eopXD. It seems like not only one place need to have a consistent way to process intrinsic. (ex. InitIntrinsicList/createRVVIntrinsics and RVVIntrinsic::RVVIntrinsic/InitRVVIntrinsic) I'm think how to avoid mismatch implementation in the

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added a comment. TL;DR: -- - Binary size of clang increase ~200k, which is +0.07% for debug build and +0.13% for release build. - Single file compilation speed up ~33x speed up for debug build and ~8.5x speed up for release build - Regression time reduce ~10% (`ninja

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng updated this revision to Diff 400358. kito-cheng added a comment. Herald added subscribers: alextsao1999, hiraditya. Changes: - Using different approach to implement to prevent build time explosion. - build time for `SemaRVVLookup.cpp` is ~6 sec in my machine. Repository: rG

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng commandeered this revision. kito-cheng edited reviewers, added: HsiangKai; removed: kito-cheng. kito-cheng added a comment. @HsiangKai hand over this revision to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai added a comment. In D111617#3183146 , @HsiangKai wrote: > Release build: > Before this patch: > > text data bssdec hex > 115471733 7987112 443760 123902605 7629a8d ./bin/clang > > After this

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai added a comment. Release build: Before this patch: text data bssdec hex 115471733 7987112 443760 123902605 7629a8d ./bin/clang After this patch: text data bssdec hex 117568981 7996376 443760

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai added a comment. In D111617#3175167 , @craig.topper wrote: > In D111617#3060377 , @HsiangKai > wrote: > >> Although it reduces the header size, this patch will increase the binary >> size of clang.

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

craig.topper added a comment. In D111617#3060377 , @HsiangKai wrote: > Although it reduces the header size, this patch will increase the binary size > of clang. > > Debug build: > Before this patch: > > textdatabss

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai updated this revision to Diff 391948. HsiangKai added a comment. - Use unique_ptr. - Avoid to create static global constructors. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 Files:

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai updated this revision to Diff 390198. HsiangKai added a comment. Address a part of comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 Files: clang/include/clang/Basic/CMakeLists.txt

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

craig.topper added inline comments. Comment at: clang/lib/Parse/ParsePragma.cpp:511 +RISCVPragmaHandler = std::make_unique(Actions); +PP.AddPragmaHandler(RISCVPragmaHandler.get()); + } Since this is a clang specific pragma should it be `#pragma clang

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai updated this revision to Diff 31. HsiangKai added a comment. Update the implementation of getMangledName(). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 Files:

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai added a comment. The clang binary size increases about +1.4%. Compared to +2.3% previously, it is better in the implementation. The test time of lit testing under `clang/test/CodeGen/RISCV/` is 25.59s. It spends 112.07s without this patch in my local environment. Repository: rG

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai updated this revision to Diff 387489. HsiangKai added a comment. Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 Files: clang/include/clang/Basic/CMakeLists.txt

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai updated this revision to Diff 387488. HsiangKai added a comment. Check required extensions when adding the declarations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111617/new/ https://reviews.llvm.org/D111617 Files:

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai updated this revision to Diff 387472. HsiangKai added a comment. Herald added subscribers: VincentWu, luke957, mgrang. Restructure the data structure to reuse information between C intrinsics. In this way, we can have a smaller binary size and speed up the lookup for the C intrinsics.

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai added a comment. In D111617#3076994 , @rogfer01 wrote: > In D111617#3060377 , @HsiangKai > wrote: > >> Although it reduces the header size, this patch will increase the binary >> size of clang. > >

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

rogfer01 added a comment. In D111617#3060377 , @HsiangKai wrote: > Although it reduces the header size, this patch will increase the binary size > of clang. Options we can consider to mitigate this: - See if we can be more economical reprenting the

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai added a comment. As discussed in D110684 , developers complain not only compile time, but also binary size & memory usage caused by RVV intrinsics. We need to consider binary size, too. Is there other way to handle it? Or we should go back to think

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai added a comment. Although it reduces the header size, this patch will increase the binary size of clang. Debug build: Before this patch: textdatabss dec hex filename 263892591 10838284500232

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:923 + + const RVVIntrinsicInfo *Intrinsic = std::find_if( + std::begin(RVVIntrinsicInfos), std::end(RVVIntrinsicInfos), kito-cheng wrote: > rogfer01 wrote: > > Not for this patch: I

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

kito-cheng added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:923 + + const RVVIntrinsicInfo *Intrinsic = std::find_if( + std::begin(RVVIntrinsicInfos), std::end(RVVIntrinsicInfos), rogfer01 wrote: > Not for this patch: I think this table may

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

rogfer01 added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:923 + + const RVVIntrinsicInfo *Intrinsic = std::find_if( + std::begin(RVVIntrinsicInfos), std::end(RVVIntrinsicInfos), Not for this patch: I think this table may be a bit large so

[PATCH] D111617: [RISCV] Lazily add RVV C intrinsics.

HsiangKai created this revision. HsiangKai added reviewers: khchen, craig.topper, rogfer01, kito-cheng. Herald added subscribers: achieveartificialintelligence, StephenFan, vkmr, frasercrmck, dexonsmith, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX,