[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.
khchen marked an inline comment as done.
khchen added inline comments.
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:700
+for (auto Idx : CTypeOrder) {
+ if (Seen.count(Idx))
+PrintFatalError(
craig.topper wrote:
> You can use
>
> ```
> if (!Seen.insert(Idx).second)
> PrintFatalError
> ```
>
> This avoids walking the set twice.
>
Fixed directly in upstream patch. Thanks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98388/new/
https://reviews.llvm.org/D98388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.
craig.topper accepted this revision. craig.topper added a comment. This revision is now accepted and ready to land. LGTM other than that one comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98388/new/ https://reviews.llvm.org/D98388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.
craig.topper added inline comments.
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:700
+for (auto Idx : CTypeOrder) {
+ if (Seen.count(Idx))
+PrintFatalError(
You can use
```
if (!Seen.insert(Idx).second)
PrintFatalError
```
This avoids walking the set twice.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98388/new/
https://reviews.llvm.org/D98388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.
khchen added inline comments. Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:687 + +unsigned Skew = 0; +if (HasMaskedOffOperand) craig.topper wrote: > ``` > unsigned Skew = HasMaskedOffOperand ? 1 : 0; > ``` > > unless this needs to get more complicated in a future patch? > No. thanks. Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:695 +// Verify the result of CTypeOrder has legal value. +if (std::upper_bound(CTypeOrder.begin(), CTypeOrder.end(), + CTypeOrder.size() - 1) != CTypeOrder.end()) craig.topper wrote: > std::upper_bound requires a list to be sorted. It tells you the upper > location of where the value belongs in the sorted sequence. std::max_element > can tell you the largest value in an unsorted range. thanks for point out my error. Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:699 + "The index of PermuteOperand is bigger than the operand number"); +if (std::unique(CTypeOrder.begin(), CTypeOrder.end()) != CTypeOrder.end()) + PrintFatalError( craig.topper wrote: > std::unique only compares adjacent values. As far it is concerned "[1, 0, 1]" > is unique because the adjacent values are different. To check for duplicates > I think you need to sort it first and then you want std::adjacent_find rather > than std::unique. std::unique modifies the range and shifts them down. > std::adjacent_find just tells you were the duplicate is. Another option is to > iterate and use a set to keep track of values you already saw. thanks, I prefer the latter. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98388/new/ https://reviews.llvm.org/D98388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.
craig.topper added inline comments. Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:687 + +unsigned Skew = 0; +if (HasMaskedOffOperand) ``` unsigned Skew = HasMaskedOffOperand ? 1 : 0; ``` unless this needs to get more complicated in a future patch? Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:695 +// Verify the result of CTypeOrder has legal value. +if (std::upper_bound(CTypeOrder.begin(), CTypeOrder.end(), + CTypeOrder.size() - 1) != CTypeOrder.end()) std::upper_bound requires a list to be sorted. It tells you the upper location of where the value belongs in the sorted sequence. std::max_element can tell you the largest value in an unsorted range. Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:699 + "The index of PermuteOperand is bigger than the operand number"); +if (std::unique(CTypeOrder.begin(), CTypeOrder.end()) != CTypeOrder.end()) + PrintFatalError( std::unique only compares adjacent values. As far it is concerned "[1, 0, 1]" is unique because the adjacent values are different. To check for duplicates I think you need to sort it first and then you want std::adjacent_find rather than std::unique. std::unique modifies the range and shifts them down. std::adjacent_find just tells you were the duplicate is. Another option is to iterate and use a set to keep track of values you already saw. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98388/new/ https://reviews.llvm.org/D98388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.
khchen marked 6 inline comments as done.
khchen added inline comments.
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:689
+ skew = 1;
+for (unsigned i = 0; i < PermuteOperands.size(); ++i) {
+ if (i != PermuteOperands[i])
rogfer01 wrote:
> These are only suggestions of sanity checks we could do here:
> - I understand `PermuteOperand.size()` should be `<=` than
> `CTypeOrder.size()`.
> - Also `PermuteOperands[i] + skew` should be `<` than `CTypeOrder.size()`.
> right?
> - We could check the result is indeed a permutation (e.g. sorting a copy of
> `CTypeOrder` is equivalent to the iota above). This one might be expensive
> although the sequences are short, not sure.
> Also PermuteOperands[i] + skew should be < than CTypeOrder.size(). right
Yes.
I did the different way to do sanity checks, maybe it's better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98388/new/
https://reviews.llvm.org/D98388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.
craig.topper added inline comments.
Comment at: clang/include/clang/Basic/riscv_vector.td:225
+Ops[0] = Builder.CreateBitCast(Ops[0],
+llvm::PointerType::getUnqual(ResultType)); }],
+ ManualCodegenMask= [{
I think you can use ResultType->getPointerTo() which is a little shorter
Comment at: clang/test/CodeGen/RISCV/rvv-intrinsics/vle.c:7
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d
-target-feature +experimental-v \
+// RUN: -Werror -Wall -o - %s >/dev/null 2>%t
+// RUN: FileCheck --check-prefix=ASM --allow-empty %s <%t
Can we use 2>&1 instead of 2>%t and skip the temporary file?
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:686
+// maskedoff operand which is always in first operand.
+unsigned skew = 0;
+if (HasMaskedOffOperand)
Capitalize Skew
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98388/new/
https://reviews.llvm.org/D98388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98388: [RISCV][Clang] Add RVV vle/vse intrinsic functions.
rogfer01 added a comment.
Overall LGTM. Thanks @khchen!
Comment at: clang/include/clang/Basic/riscv_vector.td:175
+ // builtin to C/C++. It is parameter of the unmasked version without VL
+ // operand.
+ list PermuteOperands = [];
Not sure if we want to clarify that when this list is empty, the permutation is
assumed to be equivalent to `[0, 1, 2, 3, ...]`.
Comment at: clang/include/clang/Basic/riscv_vector.td:224
+IntrinsicTypes = {ResultType, Ops[1]->getType()};
+Ops[0] = Builder.CreateBitCast(Ops[0],
+llvm::PointerType::getUnqual(ResultType)); }],
I think you may want to indent this, and then lines 228 and 248 like you
indented line 252.
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:689
+ skew = 1;
+for (unsigned i = 0; i < PermuteOperands.size(); ++i) {
+ if (i != PermuteOperands[i])
These are only suggestions of sanity checks we could do here:
- I understand `PermuteOperand.size()` should be `<=` than `CTypeOrder.size()`.
- Also `PermuteOperands[i] + skew` should be `<` than `CTypeOrder.size()`.
right?
- We could check the result is indeed a permutation (e.g. sorting a copy of
`CTypeOrder` is equivalent to the iota above). This one might be expensive
although the sequences are short, not sure.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98388/new/
https://reviews.llvm.org/D98388
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
