[PATCH] D150076: Summary: Add support for XAR instruction generation to the AArch64 backend

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks for working on this, it looks good.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:426
 
+  bool SelectXAR(SDNode *N);
+
----------------
Add 'try' to the start, like the other methods here?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4152
+
+  SDNode *N0 = cast<SDNode>(N->getOperand(0));
+  SDNode *N1 = cast<SDNode>(N->getOperand(1));
----------------
These can be SDValue


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4155
+
+  if (!(N0->getOpcode() == AArch64ISD::VSHL) || !(N1->getOpcode() == AArch64ISD::VLSHR))
+    return false;
----------------
It is clearer to use `N0->getOpcode() != AArch64ISD::VSHL ||`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4158
+
+  if (!(N0->getOperand(0)->getOpcode() == ISD::XOR) || !(N1->getOperand(0)->getOpcode() == ISD::XOR))
+    return false;
----------------
Check that N0.getOperand(0) == N1.getOperand(0), and that N0.getOperand(0).getOpcode() == ISD::XOR, to make sure they are the same XOR node.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4166
+
+  unsigned HsAmt = cast<ConstantSDNode>(N0->getOperand(1))->getZExtValue();
+  unsigned ShAmt = cast<ConstantSDNode>(N1->getOperand(1))->getZExtValue();
----------------
There is a getConstantOperandVal, which is useful when we know the value is a constant and we want to get the value.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:4173
+  SDValue Ops[] = { R1, R2, imm };
+  CurDAG->SelectNodeTo(N, AArch64::XAR, MVT::i64, Ops);
+
----------------
MVT::i64 should be VT I think, from N->getValueType(0)
You may need to use getTargetConstant for the immediate too.


================
Comment at: llvm/test/CodeGen/AArch64/xar.ll:10
+; SHA3-NEXT:    mov w8, #54 // =0x36
+; SHA3-NEXT:    xar v0.2d, v0.2d, v1.2d, w8
+; SHA3-NEXT:    ret
----------------
I believe the #54 should be part of the instruction


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150076