This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Lazily add RVV C intrinsics.
ClosedPublic

Authored by kito-cheng on Oct 12 2021, 1:01 AM.

Details

Summary

Leverage the method OpenCL uses that adds C intrinsics when the lookup
failed. There is no need to define C intrinsics in the header file any
more. It could help to avoid the large header file to speed up the
compilation of RVV source code. Besides that, only the C intrinsics used
by the users will be added into the declaration table.

This patch is based on https://reviews.llvm.org/D103228 and inspired by
OpenCL implementation.

Experimental Results

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 for debug build and ~8.5x for release build
  • Regression time reduce ~10% (ninja check-all, enable all targets)
Header size change
       |      size |     LoC |
------------------------------
Before | 4,434,725 |  69,749 |
After  |     6,140 |     162 |
Single File Compilation Time

Testcase:

#include <riscv_vector.h>

vint32m1_t test_vadd_vv_vfloat32m1_t(vint32m1_t op1, vint32m1_t op2, size_t vl) {
  return vadd(op1, op2, vl);
}
Debug build:

Before:

real    0m19.352s
user    0m19.252s
sys     0m0.092s

After:

real    0m0.576s
user    0m0.552s
sys     0m0.024s

~33x speed up for debug build

Release build:

Before:

real    0m0.773s
user    0m0.741s
sys     0m0.032s

After:

real    0m0.092s
user    0m0.080s
sys     0m0.012s

~8.5x speed up for release build

Regression time

Note: the failed case is tools/llvm-debuginfod-find/debuginfod.test which is unrelated to this patch.

Debug build

Before:

Testing Time: 1358.38s
  Skipped          :    11
  Unsupported      :   446
  Passed           : 75767
  Expectedly Failed:   190
  Failed           :     1

After

Testing Time: 1220.29s
  Skipped          :    11
  Unsupported      :   446
  Passed           : 75767
  Expectedly Failed:   190
  Failed           :     1
Release build

Before:

Testing Time: 381.98s
  Skipped          :    12
  Unsupported      :  1407
  Passed           : 74765
  Expectedly Failed:   176
  Failed           :     1

After:

Testing Time: 346.25s
  Skipped          :    12
  Unsupported      :  1407
  Passed           : 74765
  Expectedly Failed:   176
  Failed           :     1
Binary size of clang
Debug build

Before

   text    data     bss     dec     hex filename
335261851       12726004         552812 348540667       14c64efb        bin/clang

After

   text    data     bss     dec     hex filename
335442803       12798708         552940 348794451       14ca2e53        bin/clang

+253K, +0.07% code size

Release build

Before

   text    data     bss     dec     hex filename
144123975       8374648  483140 152981763       91e5103 bin/clang

After

   text    data     bss     dec     hex filename
144255762       8447296  483268 153186326       9217016 bin/clang

+204K, +0.13%

Authored-by: Kito Cheng <kito.cheng@sifive.com>
Co-Authored-by: Hsiangkai Wang <kai.wang@sifive.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 future, because we will support a lot of new builtin with tail and mask policy...

clang/lib/Sema/SemaRVVLookup.cpp
92

why do we need to declare Name as std::string here but RVVIntrinsicRecord use const char*?

93

Nit: I think we use the overload terminology rather than generic.

360–372

IIUC, above code initialize the BuiltinName, Name and MangledName same with RVVIntrinsic::RVVIntrinsic did, right?
If yes, I think we need to have some comment note that.

clang/utils/TableGen/RISCVVEmitter.cpp
85

It will be good to have a description about why we need to have custom SemaLookup.

93

missed comment?

98–99

Forget to remove this code?

102

Could we have comment for signature table.
IIUC, it stores ProtoSeq, ProtoMaskSeq, SuffixProto and MangledSuffixProto information in each entry, and SemaRecord use the index to get the information from signature table, right?

627

Use constant reference to access range-based loop?

634

Don’t Use Braces on Simple Single-Statement Bodies of if/else/loop Statements.
Please check your code again.

llvm/include/llvm/Support/RISCVVIntrinsicUtils.h
23 ↗(On Diff #400358)

I think using typed enums is clearer because we would use TypeRangeMask to record supported basic types.
It should have the same type with TypeRangeMask.

84 ↗(On Diff #400358)

different naming rule and initialize way comparing to RVVBasicTypeMaxOffset

Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 2:00 AM
kito-cheng updated this revision to Diff 426281.May 1 2022, 2:42 AM
kito-cheng marked 2 inline comments as done.

Changes:

  • Split out refactor part to D124730.
  • Add more comment.
kito-cheng updated this revision to Diff 426284.May 1 2022, 3:50 AM
kito-cheng marked 4 inline comments as done.

Changes:

  • Add more comments.
kito-cheng updated this revision to Diff 426285.May 1 2022, 3:53 AM
kito-cheng marked 2 inline comments as done.

Changes:

  • Minor tweak.
clang/lib/Sema/SemaRVVLookup.cpp
92

RVVIntrinsicRecord::Name is raw name of a intrinsic, RVVIntrinsicDef::Name is expanded with type infos, e.g. RVVIntrinsicRecord::Name is vadd and RVVIntrinsicDef::Name is vadd_vv_i32m1.

93

Updated.

360–372

More comment added.

kito-cheng marked 3 inline comments as done.May 1 2022, 3:56 AM
kito-cheng edited the summary of this revision. (Show Details)
kito-cheng edited the summary of this revision. (Show Details)May 1 2022, 3:58 AM

Do we need to have some tests in clang/test/PCH/ for new #pragma?

clang/lib/Sema/SemaLookup.cpp
932

Don’t Use Braces on Simple Single-Statement Bodies.

clang/lib/Support/RISCVVIntrinsicUtils.cpp
903–905

maybe this changed should be in another NFC patch.

clang/utils/TableGen/RISCVVEmitter.cpp
370

maybe all renaming stuffs should be in another NFC patch.

480

I feel little tricky to checking the name here. what do you mean they are handled by riscv_vector.h?
do you mean they have vsetvl_macro:RVVHeader?

621

I'm thinking is it possible to have an unittest or test to make sure we won't screw up in the future implementation?
Is it possible to have unittest to test implement really have sync correctly?
Is it easy to debug the mismatch problem during implementation without any new test added?
We will add a new implementation (really cool speed up and meaningful improvement), but unfortunately we don't have any tests, that make me a little hesitating...

What do you think?

kito-cheng marked 5 inline comments as done.

Changes:

  • Split out several NFC changes to individual NFC patchs.
  • Moving most code emission logic into RISCVVIntrinsicUtils to prevent require sync manually.
  • PCH support is WIP, will update soon.
kito-cheng added inline comments.May 24 2022, 9:06 AM
clang/utils/TableGen/RISCVVEmitter.cpp
480

Yeah, they are defined in riscv_vector.h like this:

#define vsetvl_e8mf8(avl) __builtin_rvv_vsetvli((size_t)(avl), 0, 5)
#define vsetvl_e8mf4(avl) __builtin_rvv_vsetvli((size_t)(avl), 0, 6)
#define vsetvl_e8mf2(avl) __builtin_rvv_vsetvli((size_t)(avl), 0, 7)
#define vsetvl_e8m1(avl) __builtin_rvv_vsetvli((size_t)(avl), 0, 0)
#define vsetvl_e8m2(avl) __builtin_rvv_vsetvli((size_t)(avl), 0, 1)
...
621

Change to another way to preventing sync those file manually, thanks for point out that!

khchen added inline comments.May 25 2022, 2:32 AM
clang/lib/Sema/SemaRVVLookup.cpp
175

Those code logic need to sync with createRVVIntrinsics, maybe we could add more comment address that.

clang/utils/TableGen/RISCVVEmitter.cpp
91

and also init SemaRecords?

93

I think it should be "Create all intrinsics record and SemaSignatureTable from SemaRecords"?

99

/// Construct a compressed signature table from SemaRecords which is used for createSema.
maybe better.

103

This is ambiguous of naming and comment because it also insert signature into SemaSignatureTable if not found in the table.

597

maybe we need an assert to check SemaRecords is not empty.

637

we only need SemaRecords initialization part of createRVVIntrinsics, right?

639

createRVVIntrinsicRecord also init SemaSignatureTable implicitly.

Changes:

kito-cheng marked 7 inline comments as done.Jun 30 2022, 2:47 AM
khchen accepted this revision.Jun 30 2022, 8:51 AM

LGTM. Other than that last comments.

clang/utils/TableGen/RISCVVEmitter.cpp
77

please use ArrayRef

94

maybe SemaRecords could have default argument as nullptr.

98

please use ArrayRef

185

Does it mean empty Signature always at 0?
If yes, maybe we could check the table from Index = 1 in below loop?

This revision is now accepted and ready to land.Jun 30 2022, 8:51 AM

@craig.topper @rogfer01 - do you have other comments?

craig.topper added inline comments.Jun 30 2022, 9:32 AM
clang/include/clang/Basic/TokenKinds.def
911

Why only 2 periods at the end. Should be 3 like on like 908 or in the middle of line 903

clang/include/clang/Sema/Sema.h
1585

funtion -> functions

clang/include/clang/Support/RISCVVIntrinsicUtils.h
110

I think it's more conventional to define operator== as PD.PT == PT && PD.VTM == VTM && PD.TM == TM and operator!= as !(*this == PD)

111

This can be written as

return std::tie(PD.PT, PD.VTM, PD.TM) > std::tie(PT, VTM, TM);

Though it's still surprising that PD is on the left. This is operator> but the implementation looks more like operator<.

355

"if can" -> "if it can"

386

Missing period at end of comment.

clang/lib/Parse/ParsePragma.cpp
3966

Drop parenthese arounds (!II->isStr("intrinsic"))

3974

Drop parentheses around (!II->isStr("vector"))

clang/lib/Sema/CMakeLists.txt
49

These are supposed to be in alphabetical order

clang/lib/Sema/SemaRISCVVectorLookup.cpp
68 ↗(On Diff #441320)

Can this use makeArrayRef?

117 ↗(On Diff #441320)

Drop curly braces

122 ↗(On Diff #441320)

Drop curly braces

262 ↗(On Diff #441320)

Drop curly braces

277 ↗(On Diff #441320)

Drop curly braces

311 ↗(On Diff #441320)

Drop curly braces

364 ↗(On Diff #441320)

Drop curly braces

382 ↗(On Diff #441320)

"It's not an RVV intrinsic."

389 ↗(On Diff #441320)

Should this be a member of Sema instead of a function static? RVVIntrinsicManager holds a reference to ASTContext but will outlive it.

clang/utils/TableGen/RISCVVEmitter.cpp
76

Is hsignature a typo?

490

Drop curly braces

craig.topper added inline comments.Jun 30 2022, 9:32 AM
clang/utils/TableGen/RISCVVEmitter.cpp
496

Drop curly braces

519

This lambda doesn't seem to provide much value.

What's wrong with

SR.Suffix = parsePrototypes(SuffixProto);
SR.OverloadedSuffix = parsePrototypes(OverloadedSuffixProto);
craig.topper requested changes to this revision.Jun 30 2022, 9:33 AM
craig.topper added a reviewer: aaron.ballman.
This revision now requires changes to proceed.Jun 30 2022, 9:33 AM
kito-cheng marked 24 inline comments as done.

Changes:

clang/include/clang/Support/RISCVVIntrinsicUtils.h
111

Rewrite as operator> and updated the use site, thank!

clang/utils/TableGen/RISCVVEmitter.cpp
185

Actually empty signature could be indicate into any index, we have hold length when we emit the index.

Add comment to mention that.

kito-cheng marked an inline comment as done.Jul 1 2022, 12:42 AM

Oh, have one comment not address yet

Some drive-by comments from the peanut gallery.

clang/lib/Parse/ParsePragma.cpp
3969

It's fine to warn on this, but then you need to eat tokens until the end of directive is found so that parsing recovery is correct. e.g.,

#pragma clang riscv int i = 12;

See HandlePragmaAttribute() for an example (though you'll look for eod instead of eof).

3977

Same here.

3984

And here as well.

clang/lib/Sema/SemaRISCVVectorLookup.cpp
100 ↗(On Diff #441610)

I almost hate to ask, but... long double? Any of the 16-bit float types?

111 ↗(On Diff #441610)

Might as well handle Invalid and then drop the default entirely so it's a fully covered switch.

121 ↗(On Diff #441610)

Double-checking -- do you have to care about references as well?

185–186 ↗(On Diff #441610)

Given that we're bit twiddling with this, I'd feel more comfortable if this was unsigned int rather than int (same for BaseTypeI).

223–225 ↗(On Diff #441610)
227–229 ↗(On Diff #441610)

You should spell the type explicitly here instead of using auto.

235–237 ↗(On Diff #441610)
299–301 ↗(On Diff #441610)

Spell out the types instead of using auto.

322 ↗(On Diff #441610)

No need to calculate this, we already know.

328 ↗(On Diff #441610)

Naming style fix.

342 ↗(On Diff #441610)

Spell out type name.

358 ↗(On Diff #441610)

Spell out the type.

clang/utils/TableGen/RISCVVEmitter.cpp
41
74
184–185

Pretty sure you can go with this instead.

217–222
kito-cheng updated this revision to Diff 441684.Jul 1 2022, 7:00 AM

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.
kito-cheng marked an inline comment as done.Jul 1 2022, 7:00 AM

Just nits from me.

clang/include/clang/Sema/RISCVIntrinsicManager.h
9 ↗(On Diff #441684)

handles

clang/lib/Sema/SemaRISCVVectorLookup.cpp
49 ↗(On Diff #441684)

Indices (or Indexes)?

218 ↗(On Diff #441684)

Drop curly braces

185–186 ↗(On Diff #441610)

+1

clang/utils/TableGen/RISCVVEmitter.cpp
47

Comment is plural, variable is singular.

95

all records, plural?

484

if (!SemaRecords) continue;? Might make things a little more readable.

566

I assume the compiler's able to avoid recomputing Out.back() multiple times? We could take a reference to Out.back() and use that, just in case?

kito-cheng updated this revision to Diff 441692.Jul 1 2022, 7:49 AM
kito-cheng marked 18 inline comments as done.

Changes:

  • Address @aaron.ballman’s comment
    • Add 2 new testcase:
      • riscv-bad-intrnisic-pragma.c
      • riscv-intrnisic-pragma.c
clang/lib/Parse/ParsePragma.cpp
3969

Seems like it already work correctly, and I saw other HandlePragma also just return? I add a testcase to make sure it work.

clang/lib/Sema/SemaRISCVVectorLookup.cpp
100 ↗(On Diff #441610)

Have 16 bit floating below, but we don't support long double in our intrinsic for now, add an assertion to make sure.

121 ↗(On Diff #441610)

We don't have any references type in argument type, so we don't care about that.

aaron.ballman added inline comments.Jul 1 2022, 7:56 AM
clang/lib/Parse/ParsePragma.cpp
3969

Ah, you're right, it's the *other* form of pragma handling that needs to do that dance, sorry for the noise but thank you for the additional test coverage!

clang/lib/Sema/SemaRISCVVectorLookup.cpp
100 ↗(On Diff #441610)

Very glad to hear about long double, but I was unclear on the 16-bit float, I was more wondering if you need to differentiate between Float16Ty, BFloat16Ty, and HalfTy since those will all have the same bit widths but be different types.

121 ↗(On Diff #441610)

Excellent, thank you

kito-cheng updated this revision to Diff 442025.Jul 4 2022, 1:24 AM
kito-cheng marked 9 inline comments as done.

Changes:

clang/lib/Sema/SemaRISCVVectorLookup.cpp
100 ↗(On Diff #441610)

RISC-V vector intrinsic only support _Float16(Float16Ty) for now, __fp16(HalfTy, half in OpenCL) won't support, BFloat16Ty will be another ScalarTypeKind like ScalarTypeKind::BFloat, we didn't add yet since we don't have any bfloat16 instruction in RISC-V extension now.

clang/utils/TableGen/RISCVVEmitter.cpp
566

Checked code gen with following program, got almost same code gen:

#include <vector>
struct X{
    int a;
    int b;
    int c;
};

#ifdef BACK
void foo(std::vector<X> &Out){
    X x;
    Out.emplace_back(x);
    Out.back().a =12;
    Out.back().b =23;
    Out.back().c =30;
}
#else
void foo2(std::vector<X> &Out){
    Out.emplace_back(X());
    X &x = Out.back();
    x.a =12;
    x.b =23;
    x.c =30;
}
#endif

But I think later one might be more readable, let me tweak that.

kito-cheng updated this revision to Diff 442429.Jul 5 2022, 7:46 PM

Changes:

  • Less invasive way to fix this issue.
kito-cheng updated this revision to Diff 442431.Jul 5 2022, 7:49 PM

Changes:

Restore the patch, I just accidentally updated wrong revision here...

craig.topper added inline comments.Jul 8 2022, 10:26 AM
clang/test/Sema/riscv-bad-intrnisic-pragma.c
1 ↗(On Diff #442431)

this test file name is misspelled

clang/test/Sema/riscv-intrnisic-pragma.c
1 ↗(On Diff #442431)

test file name is misspelled

craig.topper added inline comments.Jul 8 2022, 10:27 AM
clang/include/clang/Support/RISCVVIntrinsicUtils.h
17

Do we need the header or is a forward declaration enough?

kito-cheng marked 3 inline comments as done.Jul 13 2022, 1:51 AM

Changes:

  • Correct filename for testcases.
  • Use forward declaration for llvm::raw_ostream

@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 :)

aaron.ballman accepted this revision.Jul 25 2022, 7:15 AM

LGTM, only minor nits found (feel free to ignore any that you disagree with).

clang/include/clang/Support/RISCVVIntrinsicUtils.h
339
355
393
clang/lib/Sema/SemaRISCVVectorLookup.cpp
32 ↗(On Diff #444188)
40 ↗(On Diff #444188)
131 ↗(On Diff #444188)

You can drop all uses of clang:: in this file.

181–187 ↗(On Diff #444188)

Please don't use auto when the type is not spelled out in the initialization.

307–308 ↗(On Diff #444188)
clang/utils/TableGen/RISCVVEmitter.cpp
63
kito-cheng marked 9 inline comments as done.Jul 26 2022, 12:39 AM

@aaron.ballman thanks for your review!

clang/include/clang/Support/RISCVVIntrinsicUtils.h
393

That the term used in RVV spec, so I keep this as NF :)

This revision was not accepted when it landed; it landed in state Needs Review.Jul 26 2022, 12:48 AM
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.