[PATCH] D155565: [AArch64] SelectionDAG Funnel Shift Lowering

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Remember to upload with context. It can make the patches easier to read, thanks.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5805
 
+// Lower funnel shift right if possible
+static SDValue LowerFunnelShift(SDValue Op, SelectionDAG &DAG) {
----------------
Can you expand this comment a little to say that it treats fshr with constant shifts as legal, fshl is converted to fshr and the others are expanded.

Does it need to check that the constant is within the right range? Or does that happen automatically somehow?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5810
+  if (auto *ShiftNo = dyn_cast<ConstantSDNode>(Shifts)) {
+    SDLoc dl(Op);
+    MVT VT = Op.getSimpleValueType();
----------------
`dl` is used in several places in this file, but in general llvm prefers CamelCase for variable names, which for this would make it `DL`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5814
+    if (Op.getOpcode() == ISD::FSHL) {
+      long unsigned int newShiftNo =
+          VT.getFixedSizeInBits() - ShiftNo->getZExtValue();
----------------
newShiftNo -> NewShiftNo


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

https://reviews.llvm.org/D155565