[PATCH] [ARM] Fix issue with SMLAL (Signed Multiply Accumulate Long) lowering

[Tim Northover]

Hi Jyoti,

> The rationale behind my thinking was that there would never be a case where we need to update the same LoMul and LowAdd variables consecutively.


I agree with that, but think the problem was really how we were deciding which one we should be updating. This fix looks more likely, but still improvable:


REPOSITORY
  rL LLVM

================
Comment at: ../llvm/lib/Target/ARM/ARMISelLowering.cpp:8081
@@ -8078,3 +8080,3 @@
 
-  if (AddcOp0->getOpcode() == Opc) {
+  if ((AddcOp0->getOpcode() == Opc) && AddcOp0.getNode() == MULOp.getNode()) {
     LoMul = &AddcOp0;
----------------
I think we actually want this check to be

    if (AddcOp0 == MULOp.getValue(0))

The existing weird opcode logic looks like it was a buggy attempt to work around the problem that ADDE and ADDC use different halves of *MUL_LOHI. I suspect you could get wrong codegen if you could arrange that the low half of SMUL_LOHI gets fed into the ADDE and the high half into the ADDC.

If that's right, we also want to check that the AddeOp is using the MULOp->getValue(1) part; and while we're at it, this check just below has become redundant:

    if (LoMul->getNode() != HiMul->getNode())
      return SDValue();

http://reviews.llvm.org/D6998

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