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

[Matthias Braun]

- Please upload your patches with context, see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
- First: I think this patch is incorrect: I haven't tried it but reading the code it looks like it does not check that the UMUL/SMUL does not overflow, in which case using an UMLAL/SMLAL changes the semantics. It is not obvious to me how the code prohibits that, it looks like it accepts any Constant, SRA, CopyFromReg as input to the UMUL/SMUL. But as a simple example (UMUL 0xffffffff, 0xffffffff) - let's assume it is not constant folded - is accepted here because it has two constant inputs but does overflow and cannot be combined with an addc/adde to an UMLAL.
- A test for SMLAL would be nice.


REPOSITORY
  rL LLVM

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:7946-7951
@@ -7945,6 +7945,8 @@
 
-  if (Subtarget->isThumb1Only()) return SDValue();
+  if (Subtarget->isThumb1Only())
+    return SDValue();
 
   // Only perform the checks after legalize when the pattern is available.
-  if (DCI.isBeforeLegalize()) return SDValue();
+  if (DCI.isBeforeLegalize())
+    return SDValue();
 
----------------
These changes are unrelated and should be left out of this patch.

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:7992
@@ -7988,3 +7991,3 @@
   // Look for the glued ADDE.
-  SDNode* AddeNode = AddcNode->getGluedUser();
+  SDNode *AddeNode = AddcNode->getGluedUser();
   if (!AddeNode)
----------------
unrelated change.

================
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;
----------------
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.

================
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) {
----------------
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.

================
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;
+    }
 
----------------
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.

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:8050-8051
@@ -8031,1 +8049,4 @@
+      IsLeftOperandMUL = true;
+    if (MULOp == SDValue())
+      return SDValue();
 
----------------
This could be moved into the then part of the preceding if.

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:8061-8064
@@ -8037,1 +8060,6 @@
 
+    if (IsLeftOperandMUL)
+      HiAdd = &AddeOp1;
+    else
+      HiAdd = &AddeOp0;
+  }
----------------
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).

================
Comment at: ../../llvm/lib/Target/ARM/ARMISelLowering.cpp:8092
@@ -8062,3 +8091,3 @@
 
-  SDValue MLALNode =  DAG.getNode(FinalOpc, SDLoc(AddcNode),
+  SDValue MLALNode = DAG.getNode(FinalOpc, SDLoc(AddcNode),
                                  DAG.getVTList(MVT::i32, MVT::i32), Ops);
----------------
unrelated change. More unrelated changed following.

http://reviews.llvm.org/D9881

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