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

[Aaron Ballman via Phabricator via llvm-commits]

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 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`).
> Seems like it already work correctly, and I saw other HandlePragma also just return? I add a testcase to make sure it work.
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!


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:100
+    switch (Type->getElementBitwidth()) {
+    case 64:
+      QT = Context.DoubleTy;
----------------
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.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:121
+  // Transform the type to a pointer as the last step, if necessary.
+  if (Type->isPointer())
+    QT = Context.getPointerType(QT);
----------------
kito-cheng wrote:
> aaron.ballman wrote:
> > Double-checking -- do you have to care about references as well?
> We don't have any references type in argument type, so we don't care about that.
Excellent, thank you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111617