This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][Clang] Add RVV vle/vse intrinsic functions.
ClosedPublic

Authored by khchen on Mar 10 2021, 8:14 PM.

Details

Summary

Add new field PermuteOperands to mapping different operand order between C/C++ API and clang builtin.

Authored-by: Roger Ferrer Ibanez <rofirrim@gmail.com>
Co-Authored-by: Hsiangkai Wang <kai.wang@sifive.com>
Co-Authored-by: Zakk Chen <zakk.chen@sifive.com>

Diff Detail

Event Timeline

khchen created this revision.Mar 10 2021, 8:14 PM
khchen requested review of this revision.Mar 10 2021, 8:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 8:14 PM
khchen edited the summary of this revision. (Show Details)Mar 10 2021, 10:40 PM

Overall LGTM. Thanks @khchen!

clang/include/clang/Basic/riscv_vector.td
175

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

224

I think you may want to indent this, and then lines 228 and 248 like you indented line 252.

clang/utils/TableGen/RISCVVEmitter.cpp
689

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.
craig.topper added inline comments.Mar 11 2021, 5:49 PM
clang/include/clang/Basic/riscv_vector.td
225

I think you can use ResultType->getPointerTo() which is a little shorter

clang/test/CodeGen/RISCV/rvv-intrinsics/vle.c
7

Can we use 2>&1 instead of 2>%t and skip the temporary file?

clang/utils/TableGen/RISCVVEmitter.cpp
686

Capitalize Skew

khchen marked 6 inline comments as done.Mar 12 2021, 8:59 AM
khchen added inline comments.
clang/utils/TableGen/RISCVVEmitter.cpp
689

Also PermuteOperands[i] + skew should be < than CTypeOrder.size(). right

Yes.
I did the different way to do sanity checks, maybe it's better.

khchen updated this revision to Diff 330264.Mar 12 2021, 9:00 AM
khchen marked an inline comment as done.

address craig.topper and rogfer01's suggestions, thanks!

craig.topper added inline comments.Mar 15 2021, 2:02 PM
clang/utils/TableGen/RISCVVEmitter.cpp
687
unsigned Skew = HasMaskedOffOperand ? 1 : 0;

unless this needs to get more complicated in a future patch?

695

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.

699

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.

khchen updated this revision to Diff 331277.Mar 17 2021, 8:34 AM
khchen marked 3 inline comments as done.

address Craig's comments, thanks.

khchen added inline comments.Mar 17 2021, 8:59 AM
clang/utils/TableGen/RISCVVEmitter.cpp
687

No. thanks.

695

thanks for point out my error.

699

thanks, I prefer the latter.

craig.topper added inline comments.Mar 17 2021, 12:18 PM
clang/utils/TableGen/RISCVVEmitter.cpp
699

You can use

if (!Seen.insert(Idx).second)
  PrintFatalError

This avoids walking the set twice.

craig.topper accepted this revision.Mar 17 2021, 12:20 PM

LGTM other than that one comment.

This revision is now accepted and ready to land.Mar 17 2021, 12:20 PM
This revision was automatically updated to reflect the committed changes.
khchen marked an inline comment as done.Mar 17 2021, 8:34 PM
khchen added inline comments.
clang/utils/TableGen/RISCVVEmitter.cpp
699

Fixed directly in upstream patch. Thanks.