[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
[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.
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/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 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: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.
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.
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