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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. | |
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. |
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? |
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. |
llvm/lib/Target/VE/VEISelDAGToDAG.cpp | ||
---|---|---|
76–77 | I will do. Thanks. |
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.
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? |
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. |
llvm/lib/Target/VE/VEISelDAGToDAG.cpp | ||
---|---|---|
76–77 | I see. |
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.