[PATCH] D76822: [VE] Update lea/load/store instructions

[Simon Moll via Phabricator via llvm-commits]

simoll added a comment.

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



================
Comment at: llvm/lib/Target/VE/VEISelDAGToDAG.cpp:84
+    return false;
+  } else if (matchADDRrr(Addr, LHS, RHS)) {
+    if (matchADDRri(RHS, Index, Offset)) {
----------------
else after return


================
Comment at: llvm/lib/Target/VE/VEISelDAGToDAG.cpp:88
+      return true;
+    } else if (matchADDRri(LHS, Base, Offset)) {
+      Index = RHS;
----------------
else after return


================
Comment at: llvm/lib/Target/VE/VEISelDAGToDAG.cpp:159
+
   if (Addr.getOpcode() == ISD::ADD) {
     if (Addr.getOperand(0).getOpcode() == VEISD::Lo ||
----------------
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.


================
Comment at: llvm/lib/Target/VE/VEISelDAGToDAG.cpp:160
   if (Addr.getOpcode() == ISD::ADD) {
-    if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Addr.getOperand(1)))
-      if (isInt<13>(CN->getSExtValue()))
-        return false; // Let the reg+imm pattern catch this!
     if (Addr.getOperand(0).getOpcode() == VEISD::Lo ||
         Addr.getOperand(1).getOpcode() == VEISD::Lo)
----------------
This is the same as line 167, can you hoist that check?


================
Comment at: llvm/lib/Target/VE/VEISelDAGToDAG.cpp:166
     return true;
+  } else if (Addr.getOpcode() == ISD::OR) {
+    if (Addr.getOperand(0).getOpcode() == VEISD::Lo ||
----------------
else after return


================
Comment at: llvm/lib/Target/VE/VEInstrInfo.td:677
+def : Pat<(add I64:$base, simm32:$disp), (LEArii $base, 0, (LO32 $disp))>;
+def : Pat<(add (add I64:$base, simm7:$idx), simm32:$disp),
+          (LEArii $base, (LO7 $idx), (LO32 $disp))>;
----------------
There are a few redundant cases here due to possible operand orderings in the nested `add`s.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76822/new/

https://reviews.llvm.org/D76822