[PATCH] [PATCH][ARM] Fix issue with UMLAL (unsigned multiple accumulate long) lowering

[Jyoti Allur]

Hi Matthias,
I had abandoned this change due to out of context changes shown after applying clang-format and some other issue.
To clarify, Constant, SRA, CopyFromReg are possible input nodes to ADDE node. 
I was trying to cover cases where ISD::MUL, ISD::ADDC, ISD::ADDE can be combined to a S/UMLAL where in ISD::MUL operands are less 8-bit or 16-bits operands. In these cases result can stored in a in single 32-bit register. Hence in such cases, none of ADDE 's operand will store result from ISD:MUL. Instead, one of the ADDE's operand will hold 32:63 bits of 64-bit accumulate value, while the 0:31 bits of the accumulate value is stored in one of the operands of ADDC.

for example: in the assembly below, 
R1 - ACC [ 32:63 ] 
R0 - ACC [ 0:31 ]

  foo:
          ldrb    r2, [r2]
          ldrh    r3, [r3]
          mul     r2, r3, r2
          adds    r0, r2, r0
          adc     r1, r1, #0
          bx      lr

I agree, overflow cases were not handled yet. Could you suggest how to detect overflow ?


REPOSITORY
  rL LLVM

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:8013
@@ +8012,3 @@
+  // Figure out the high and low input values to the MLAL node.
+  SDValue *HiAdd = nullptr;
+  SDValue *LoMul = nullptr;
----------------
MatzeB wrote:
> This should be set (and is) on every path before use, leaving out the unnecessary initialization will get warnings to people that might refactor this and miss a case.
ok.

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:8014-8017
@@ +8013,6 @@
+  SDValue *HiAdd = nullptr;
+  SDValue *LoMul = nullptr;
+  SDValue *LowAdd = nullptr;
+  unsigned Opc, FinalOpc;
+  SDValue MULOp;
+  if (AddcOp0->getOpcode() == ISD::MUL || AddcOp1->getOpcode() == ISD::MUL) {
----------------
MatzeB wrote:
> Move all these down towards their definitions some of them can even be merged with their definitions because they only have one. This esp. avoids the strange shadowing of the 2nd SDValue MULOp in the then case.
ok

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:8029-8031
@@ -8022,6 +8028,5 @@
 
-  // Figure out the high and low input values to the MLAL node.
-  SDValue* HiAdd = nullptr;
-  SDValue* LoMul = nullptr;
-  SDValue* LowAdd = nullptr;
+    if (AddeOp0->getOpcode() == ISD::CopyFromReg) {
+      IsLeftOperandCopyFromReg = true;
+    }
 
----------------
MatzeB wrote:
> Remove declaration of IsLeftOperandCopyFromReg and do:
> bool IsLeftOperandCopyFromReg = (AddeOp0->getOpcode() == ISD::CopyFromReg);
> You could also inline into the if directly, but as that variable name has documentation value I'm fine with not doing that as well.
ok

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:8061-8064
@@ -8037,1 +8060,6 @@
 
+    if (IsLeftOperandMUL)
+      HiAdd = &AddeOp1;
+    else
+      HiAdd = &AddeOp0;
+  }
----------------
MatzeB wrote:
> Maybe don't bother introducing the IsLeftOperandMul variable and simply Set HiAdd = &AddeOp1/&AddeOp0 above instead of IsLeftOperandMul = true/false; (although I realize that was already strange in the previous code).
IsLeftOperandMul was probably added for code readability and understanding pruposes.

http://reviews.llvm.org/D9881

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/