This is an archive of the discontinued LLVM Phabricator instance.

[VP] Build VP SDNodes
ClosedPublic

Authored by simoll on Nov 13 2020, 9:36 AM.

Details

Summary

Translate VP intrinsics to VP_* SDNodes. The tests check whether a matching vp_* SDNode is emitted.

Diff Detail

Event Timeline

simoll created this revision.Nov 13 2020, 9:36 AM
simoll requested review of this revision.Nov 13 2020, 9:36 AM
craig.topper added inline comments.Nov 13 2020, 3:58 PM
llvm/include/llvm/IR/VPIntrinsics.def
30–87

Can OC here be OPC? That's probably a more common abbreviation for opcode.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6148

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.

simoll added inline comments.Nov 16 2020, 12:37 AM
llvm/include/llvm/IR/VPIntrinsics.def
30–87

That's upstream code - i'll rename to HANDLE_VP_TO_OPC in an NFC patch and rebase.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6148

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.
If we want to change this, we should probably do so in a separate patch that uses dyn_cast<XIntrinsic> where possible.

simoll updated this revision to Diff 305438.Nov 16 2020, 2:06 AM
simoll marked an inline comment as done.
simoll edited the summary of this revision. (Show Details)
  • Rebased onto renaming patch (OC -> OPC)
frasercrmck added inline comments.Nov 17 2020, 3:55 AM
llvm/include/llvm/IR/VPIntrinsics.def
36

typo: canonical

45

Missing ( somewhere?

47

Mismatch between EVLPOS and VLENPOS

75

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?

simoll updated this revision to Diff 306082.Nov 18 2020, 6:11 AM
simoll marked 4 inline comments as done.
  • Adressed comments
  • Cleanup
  • Rebased
simoll added inline comments.Nov 19 2020, 1:01 AM
llvm/include/llvm/IR/VPIntrinsics.def
75

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.

simoll updated this revision to Diff 307794.Nov 26 2020, 1:58 AM
simoll edited the summary of this revision. (Show Details)

Added one test for every VP SDNode type.
Ping?

simoll updated this revision to Diff 307800.Nov 26 2020, 2:20 AM

NFC. Improved tests - also checking the sdnode operand list and types

simoll updated this revision to Diff 307832.Nov 26 2020, 4:46 AM
  • Rebased
  • Make all tests use the same run command
khchen added a subscriber: khchen.Nov 26 2020, 5:48 AM
craig.topper added inline comments.Nov 30 2020, 3:36 PM
llvm/include/llvm/IR/VPIntrinsics.def
24

Should this use EVLPOS instead of VLENPOS?

40

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.

62

canonical*

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7030

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?

7055

Drop curlies.

llvm/test/CodeGen/VE/Vector/vp_add.ll
2

Can we use "not llc" instead of "|| true"?

simoll updated this revision to Diff 308624.Dec 1 2020, 5:55 AM
simoll marked 6 inline comments as done.

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.
simoll added a comment.Dec 8 2020, 1:15 AM

A friendly ping to raise this to your radar. Does this "LGTM" to you?

craig.topper accepted this revision.Dec 8 2020, 10:10 AM

LGTM with that one comment.

llvm/include/llvm/IR/VPIntrinsics.def
66

The descripton for SDOPC doesn't line up with the lines above and below

This revision is now accepted and ready to land.Dec 8 2020, 10:10 AM
simoll updated this revision to Diff 310456.Dec 9 2020, 1:35 AM
simoll marked an inline comment as done.
  • Rebased
  • Fixed indent issue.

Thanks!

This revision was landed with ongoing or failed builds.Dec 9 2020, 2:38 AM
Closed by commit rG3ffbc7935718: [VP] Build VP SDNodes (authored by simoll). · Explain Why
This revision was automatically updated to reflect the committed changes.
kaz7 added a subscriber: kaz7.Dec 9 2020, 10:23 PM

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.

kaz7 added inline comments.Dec 18 2020, 5:16 PM
llvm/include/llvm/IR/VPIntrinsics.def
134

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?