Page MenuHomePhabricator

[VE] Update lea/load/store instructions
ClosedPublic

Authored by kaz7 on Mar 25 2020, 9:27 PM.

Details

Summary

Modify lea/load/store instructions to accept disp(index, base)
style addressing mode (called ASX format). Also, uniform the
number of DAG nodes to have 3 operands for this ASX format
instructions, and update selectADDR functions to lower
appropriate MI.

Diff Detail

Event Timeline

kaz7 created this revision.Mar 25 2020, 9:27 PM
kaz7 updated this revision to Diff 252755.Mar 25 2020, 11:32 PM

Updating previous patch using clang-format.

kaz7 updated this revision to Diff 252762.Mar 26 2020, 12:47 AM

Update previous patch again by following clang-tidy suggestion.

Thank you for the patch!
I've added a few minor remarks inline. Otherwise, this looks good to go to me.

llvm/lib/Target/VE/VEISelDAGToDAG.cpp
76–77

This is a generalized version of SelectionDAG::isBaseWithConstantOffset. I suppose this code (up until line 177) should be moved into SelectionDAG, eg as SelectionDAG::isBaseWithOffset.
You can do that in a followup patch.

76–77

This is the same as line 167, can you hoist that check?

84

else after return

88

else after return

108

else after return

llvm/lib/Target/VE/VEInstrInfo.td
677

There are a few redundant cases here due to possible operand orderings in the nested adds.
You could de-duplicate those matchers with a ternary add matcher using PatFrags similar to this one: https://github.com/sx-aurora-dev/llvm-project/blob/hpce/develop/llvm/lib/Target/VE/VVPInstrInfo.td#L147
This would possibly allow you to later match not only proper add nodes but also or ones that behave like an add.

kaz7 updated this revision to Diff 253467.Mar 29 2020, 6:25 PM

Thank you for the comments. I modify source code following
those suggestions.

kaz7 marked 6 inline comments as done.Mar 29 2020, 6:29 PM
kaz7 added inline comments.
llvm/lib/Target/VE/VEISelDAGToDAG.cpp
76–77

Regarding to isBaseWithOffset, I'll try that. Should I submit them as independent patch or just append modifications on SelectionDAG in this patch?

kaz7 added a comment.Mar 29 2020, 6:33 PM

I did git ci -a --amend and arc diff. I guess it corrupts comments from reviewers. I'll try git ci -a and arc diff next time.

I did git ci -a --amend and arc diff. I guess it corrupts comments from reviewers. I'll try git ci -a and arc diff next time.

It's ok to squash all changes into one commit by amending to it. Btw, i recommend using arc diff --preview to double check that the commit is clean before submitting it to Phabricator.

llvm/lib/Target/VE/VEISelDAGToDAG.cpp
76–77

Better make it a separate patch for Phabricator after this patch is committed.

kaz7 marked 3 inline comments as done.Mar 30 2020, 1:11 AM
kaz7 added inline comments.
llvm/lib/Target/VE/VEISelDAGToDAG.cpp
76–77

I will do. Thanks.

kaz7 marked an inline comment as done.Mar 31 2020, 8:53 PM
arsenm resigned from this revision.Apr 2 2020, 9:10 AM
kaz7 added a subscriber: arsenm.Apr 2 2020, 4:39 PM

Thanks @arsenm for taking a look. I think I should add more reviewers, but not sure how to find more ppl having interest on this topic.

kaz7 added a comment.Apr 2 2020, 4:41 PM

Ping. Is there anything I can do on this patch? Thanks.

kaz7 updated this revision to Diff 254748.Apr 3 2020, 4:05 AM

Rebased to the latest upstream/master.

simoll accepted this revision.Apr 3 2020, 6:54 AM

LGTM

This revision is now accepted and ready to land.Apr 3 2020, 6:54 AM
This revision was automatically updated to reflect the committed changes.
kaz7 marked an inline comment as done.Apr 8 2020, 12:35 AM
kaz7 added inline comments.
llvm/lib/Target/VE/VEISelDAGToDAG.cpp
76–77

Today I checked other architecture implementations to consider how add isBaseWithOffset function. However, it looks like only ARM, VE, and X86 support register + register instructions, and all other architecture has difficulties to use isBaseWithOffst function even if I add it. How do you think? Should I add the function?

simoll added inline comments.Apr 8 2020, 12:44 AM
llvm/lib/Target/VE/VEISelDAGToDAG.cpp
76–77

I'd still add it to SelectionDAG. The function is not complicated but general enough to be useful for other targets.

kaz7 marked an inline comment as done.Apr 8 2020, 1:01 AM
kaz7 added inline comments.
llvm/lib/Target/VE/VEISelDAGToDAG.cpp
76–77

I see.