Translate VP intrinsics to VP_* SDNodes. The tests check whether a matching vp_* SDNode is emitted.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/IR/VPIntrinsics.def | ||
---|---|---|
27–84 | Can OC here be OPC? That's probably a more common abbreviation for opcode. | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
6140 | Instead of adding all of the enumerated values to the switch, I wonder if it might sense to use a dyn_cast<VPIntrinsic> in the default case? Same for constrained fp. |
llvm/include/llvm/IR/VPIntrinsics.def | ||
---|---|---|
27–84 | That's upstream code - i'll rename to HANDLE_VP_TO_OPC in an NFC patch and rebase. | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
6140 | I can only speak for this patch - this was born out of laziness and the vague idea that having one "uberswitch" over all intrinsics gives you better/faster code. |
llvm/include/llvm/IR/VPIntrinsics.def | ||
---|---|---|
33 | typo: canonical | |
42 | Missing ( somewhere? | |
44 | Mismatch between EVLPOS and VLENPOS | |
72 | I'm thinking of these as scopes as the documentation describes them, and it strikes me that the "INTRINSIC" and "SDNODE" scopes overlap here when using BEGIN_REGISTER_VP and END_REGISTER_VP: BEGIN_REGISTER_VP_INTRINSIC BEGIN_REGISTER_VP_SDNODE END_REGISTER_VP_INTRINSIC END_REGISTER_VP_SDNODE Is there a reason for this, or should it be balanced? BEGIN_REGISTER_VP_INTRINSIC BEGIN_REGISTER_VP_SDNODE END_REGISTER_VP_SDNODE END_REGISTER_VP_INTRINSIC | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h | ||
762 | parameter named VPIntrin like in the function def? |
llvm/include/llvm/IR/VPIntrinsics.def | ||
---|---|---|
72 | One advantage of improper nesting is that you can check easily whether scopes overlap just using macros. Eg, you can ask what is the canonical SDNode of this VPIntrinsic: BEGIN_VP_INTRINSIC BEGIN_VP_SDNODE ; <-- this SDNode belongs to the VP intrinsic of the outer scope [..] END_VP_INTRINSIC At the same time, you can go the opposite direction and look up the VPIntrinsic that has this SDNode as its canonical form: BEGIN_VP_SDNODE [..] END_VP_INTRINSIC ; <-- this intrinsic belongs to the VP SDNode of the outer scope END_VP_SDNODE (not that we were exploiting that atm..).. on the other hand nesting scopes is more intuitive but doesn't seem to have anything else speaking for it. |
llvm/include/llvm/IR/VPIntrinsics.def | ||
---|---|---|
24 | Should this use EVLPOS instead of VLENPOS? | |
37 | I think ISDOPC or something might be better than SDID. I don't think we tend to call them ids for SelectionDAG. In SelectionDAG terms, id would tend to mean the NodeID field in SDNode. | |
59 | canonical* | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp | ||
7014 | This ever expected to return an Optional with no value set? It looks like the only caller just calls getValue(). Should we just have an llvm_unreachable in the switch instead? | |
7039 | Drop curlies. | |
llvm/test/CodeGen/VE/Vector/vp_add.ll | ||
2 | Can we use "not llc" instead of "|| true"? |
Thanks for your feedback so far!
- Tests: use not --crash instead of piping to true. (Hexagon tests do the same thing).
- Say VPIntrin instead of VPI.
- Say SDOPC instead of SDID.
- Added llvm_unreachable() if no SDNode opcode is available for a given vp intrinsic.
- Fixed various cases of can(n)onical i missed in the last update.
LGTM with that one comment.
llvm/include/llvm/IR/VPIntrinsics.def | ||
---|---|---|
63 | The descripton for SDOPC doesn't line up with the lines above and below |
I noticed these new tests won't work on Release mode, so I disabled them. Please update them later.
llvm/test/CodeGen/VE/Vector/vp_add.ll | ||
---|---|---|
4 | This test won't work on Release mode since error messages don't contain temporal register name under Release mode. I disabled all tests under Release mode. Please update them later. |
llvm/include/llvm/IR/VPIntrinsics.def | ||
---|---|---|
131 | While I'm reviewing https://reviews.llvm.org/D93534, I've just noticed only this one has VP_Sub. Do you like to use VP_SUB instead? |
Should this use EVLPOS instead of VLENPOS?