[RISCV] Lazily add RVV C intrinsics.

[kito-cheng]

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 <[e-mail]>
Co-Authored-by: Hsiangkai Wang <[e-mail]>

[khchen]

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

91 ↗

(On Diff #400358)

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

92 ↗

(On Diff #400358)

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

359–371 ↗

(On Diff #400358)

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

143 ↗

(On Diff #400358)

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

152 ↗

(On Diff #400358)

missed comment?

163 ↗

(On Diff #400358)

Forget to remove this code?

171 ↗

(On Diff #400358)

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?

853 ↗

(On Diff #400358)

Use constant reference to access range-based loop?

860 ↗

(On Diff #400358)

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

[kito-cheng]

Changes:

• Split out refactor part to D124730.
• Add more comment.

[kito-cheng]

Changes:

• Add more comments.

[kito-cheng]

Changes:

• Minor tweak.

clang/lib/Sema/SemaRVVLookup.cpp

91 ↗

(On Diff #400358)

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.

92 ↗

(On Diff #400358)

Updated.

359–371 ↗

(On Diff #400358)

More comment added.

[khchen]

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

clang/lib/Sema/SemaLookup.cpp

932 ↗

(On Diff #426285)

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

clang/lib/Support/RISCVVIntrinsicUtils.cpp

884 ↗

(On Diff #426285)

maybe this changed should be in another NFC patch.

clang/utils/TableGen/RISCVVEmitter.cpp

372 ↗

(On Diff #426285)

maybe all renaming stuffs should be in another NFC patch.

480 ↗

(On Diff #426285)

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?

611 ↗

(On Diff #426285)

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]

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]

clang/utils/TableGen/RISCVVEmitter.cpp

480 ↗

(On Diff #426285)

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)
...

611 ↗

(On Diff #426285)

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

[khchen]

clang/lib/Sema/SemaRVVLookup.cpp

175 ↗

(On Diff #431646)

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

clang/utils/TableGen/RISCVVEmitter.cpp

91 ↗

(On Diff #431646)

and also init SemaRecords?

93 ↗

(On Diff #431646)

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

99 ↗

(On Diff #431646)

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

103 ↗

(On Diff #431646)

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

597 ↗

(On Diff #431646)

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

637 ↗

(On Diff #431646)

we only need SemaRecords initialization part of createRVVIntrinsics, right?

639 ↗

(On Diff #431646)

createRVVIntrinsicRecord also init SemaSignatureTable implicitly.

[kito-cheng]

Changes:

• Rebase
• Address @khchen's comment

[khchen]

LGTM. Other than that last comments.

clang/utils/TableGen/RISCVVEmitter.cpp

77 ↗

(On Diff #441320)

please use ArrayRef

110 ↗

(On Diff #441320)

maybe SemaRecords could have default argument as nullptr.

114 ↗

(On Diff #441320)

please use ArrayRef

254 ↗

(On Diff #441320)

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

[khchen]

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

[craig.topper]

clang/include/clang/Basic/TokenKinds.def

911 ↗

(On Diff #441320)

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

1590 ↗

(On Diff #441320)

funtion -> functions

clang/include/clang/Support/RISCVVIntrinsicUtils.h

110 ↗

(On Diff #441320)

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

114 ↗

(On Diff #441320)

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<.

364 ↗

(On Diff #441320)

"if can" -> "if it can"

395 ↗

(On Diff #441320)

Missing period at end of comment.

clang/lib/Parse/ParsePragma.cpp

3960 ↗

(On Diff #441320)

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

3968 ↗

(On Diff #441320)

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

clang/lib/Sema/CMakeLists.txt

49 ↗

(On Diff #441320)

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 ↗

(On Diff #441320)

Is hsignature a typo?

582 ↗

(On Diff #441320)

Drop curly braces

[craig.topper]

clang/utils/TableGen/RISCVVEmitter.cpp

588 ↗

(On Diff #441320)

Drop curly braces

611 ↗

(On Diff #441320)

This lambda doesn't seem to provide much value.

What's wrong with

SR.Suffix = parsePrototypes(SuffixProto);
SR.OverloadedSuffix = parsePrototypes(OverloadedSuffixProto);

[kito-cheng]

Changes:

• Address @craig.topper's comment
• Address @khchen's comment

clang/include/clang/Support/RISCVVIntrinsicUtils.h

114 ↗

(On Diff #441320)

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

clang/utils/TableGen/RISCVVEmitter.cpp

254 ↗

(On Diff #441320)

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]

Oh, have one comment not address yet

[aaron.ballman]

Some drive-by comments from the peanut gallery.

clang/lib/Parse/ParsePragma.cpp

3963 ↗

(On Diff #441610)

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).

3971 ↗

(On Diff #441610)

Same here.

3978 ↗

(On Diff #441610)

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 ↗

(On Diff #441610)

74 ↗

(On Diff #441610)

232–237 ↗

(On Diff #441610)

239–240 ↗

(On Diff #441610)

Pretty sure you can go with this instead.

[kito-cheng]

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.

[frasercrmck]

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 ↗

(On Diff #441684)

Comment is plural, variable is singular.

111 ↗

(On Diff #441684)

all records, plural?

578 ↗

(On Diff #441684)

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

637 ↗

(On Diff #441684)

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]

Changes:

• Address @aaron.ballman’s comment
  • Add 2 new testcase:
    • riscv-bad-intrnisic-pragma.c
    • riscv-intrnisic-pragma.c

clang/lib/Parse/ParsePragma.cpp

3963 ↗

(On Diff #441610)

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]

clang/lib/Parse/ParsePragma.cpp

3963 ↗

(On Diff #441610)

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]

Changes:

• Address @frasercrmck's comment

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

637 ↗

(On Diff #441684)

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]

Changes:

• Less invasive way to fix this issue.

[kito-cheng]

Changes:

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

[craig.topper]

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]

clang/include/clang/Support/RISCVVIntrinsicUtils.h

17 ↗

(On Diff #442431)

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

[kito-cheng]

Changes:

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

[kito-cheng]

ping

[kito-cheng]

@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]

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

clang/include/clang/Support/RISCVVIntrinsicUtils.h

346 ↗

(On Diff #444188)

362 ↗

(On Diff #444188)

400 ↗

(On Diff #444188)

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 ↗

(On Diff #444188)

[kito-cheng]

@aaron.ballman thanks for your review!

clang/include/clang/Support/RISCVVIntrinsicUtils.h

400 ↗

(On Diff #444188)

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

llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp

static void updateOperands(MachineInstr &MI, RegImmPair OldRegImm,
                            Register NewReg) {
   unsigned Opcode = MI.getOpcode();
 
   // If this pass is extended to support more instructions, the check for
   // definedness may need to be strengthened.
   assert((isCompressibleLoad(MI) || isCompressibleStore(MI)) &&
          "Unsupported instruction for this optimization.");
 
+  int SkipN = 0;
+
+  // Skip first operand for store instruction, it's operand for store value,
+  // it's unsafe to rename if offset is non-zero.
+  if (isCompressibleStore(MI) && OldRegImm.Imm != 0)
+    SkipN = 1;
+
   // Update registers
-  for (MachineOperand &MO : MI.operands())
+  for (MachineOperand &MO : drop_begin(MI.operands(), SkipN))
     if (MO.isReg() && MO.getReg() == OldRegImm.Reg) {
       // Do not update operands that define the old register.
       //
       // The new register was scavenged for the range of instructions that are
       // being updated, therefore it should not be defined within this range
       // except possibly in the final instruction.
       if (MO.isDef()) {
         assert(isCompressibleLoad(MI));

llvm/test/CodeGen/RISCV/make-compressible-for-store-address.mir

bb.0.entry:
     liveins: $x10
 
     ; CHECK-LABEL: name: foo
     ; CHECK: liveins: $x10
     ; CHECK-NEXT: {{  $}}
     ; CHECK-NEXT: renamable $x11 = ADDI $x0, 1
     ; CHECK-NEXT: $x12 = ADDI $x10, 768
     ; CHECK-NEXT: SD killed renamable $x11, $x12, 32 :: (store (s64) into %ir.1)
-    ; CHECK-NEXT: SD $x12, $x12, 40 :: (store (s64) into %ir.2)
+    ; CHECK-NEXT: SD renamable $x10, $x12, 40 :: (store (s64) into %ir.2)
     ; CHECK-NEXT: renamable $x11 = ADDI $x0, 2
     ; CHECK-NEXT: SD killed renamable $x11, killed $x12, 48 :: (store (s64) into %ir.3)
     ; CHECK-NEXT: PseudoRET
     renamable $x11 = ADDI $x0, 1
     SD killed renamable $x11, renamable $x10, 800 :: (store (s64) into %ir.1)
     SD renamable $x10, renamable $x10, 808 :: (store (s64) into %ir.2)
     renamable $x11 = ADDI $x0, 2
     SD killed renamable $x11, killed renamable $x10, 816 :: (store (s64) into %ir.3)
     PseudoRET
 
 ...