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

[Kito Cheng via Phabricator via llvm-commits]

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 hate to ask, but... `long double`? Any of the 16-bit float types?
> > Have 16 bit floating below, but we don't support long double in our intrinsic for now, add an assertion to make sure.
> 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.
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.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:637
+    Out.emplace_back(Record);
+    Out.back().Name = SR.Name.c_str();
+    Out.back().OverloadedName = SR.OverloadedName.c_str();
----------------
frasercrmck wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111617/new/

https://reviews.llvm.org/D111617