[llvm-commits] LLVM patch to implement UMLAL/SMLAL Instructions for ARM Architecture.

[Arnold Schwaighofer]

Hi Yin Ma,

I have two questions about the patch.

Question 1:

Wouldn't calling PerformADDECCombine only on ADDC nodes be sufficient?

ARMTargetLowering::PerformDAGCombine(SDNode *N,
                                               DAGCombinerInfo &DCI) const {
    switch (N->getOpcode()) {
    default: break;
+  case ISD::ADDE:
+  case ISD::ADDC:       return PerformADDECCombine(N, DCI, Subtarget);

It seems to me that when we executed PerformADDECCombine on an ADDE it 
would not succeed in doing the transform.
We are looking for patterns like (to be viewed in a monospaced font):

//    ValForLoAdd   UMUL_LOHI
//            \    / :lo    \ :hi
//             \  /          \
//   N -->      ADDC         |  ValForHiAdd
//                 \ :glue  /  /
//                  \      /  /
//                    ADDE
//

In the code we start looking for an ADD that is glued to N.
[We should use "HiAdd = N->getGluedUser()" for the following code. It 
does exactly what we want.]

+  // follow the glue value to get the second add
+  // there may be one or multiple uses, use while
+  // loop to look up the first use of the second value.
+  SDNode::use_iterator UI = N->use_begin();
+  SDNode::use_iterator UE = N->use_end();
+  SDNode* HiAdd = NULL;
+  while (UI != UE) {
+    SDUse& Nuse = UI.getUse();
+    if (Nuse.getResNo() == 1) {
+      HiAdd = Nuse.getUser();
+      break;
+    }
+    UI++;
+  }

And from that glued ADD we look for the UMUL_LOHI:

   // double check for triangle shape
+  SDValue H0 = HiAdd->getOperand(0);
+  SDValue H1 = HiAdd->getOperand(1);
+
+  // check if the values of two operands are not
+  // from the same mul_lohi node, this is possible
+  if (H0.getNode() == H1.getNode()) return SDValue();
+
+  unsigned Opc = H0->getOpcode();
+  unsigned FinalOpc;
+  if (Opc == ISD::UMUL_LOHI) FinalOpc = ARMISD::UMLAL;
+  else if (Opc == ISD::SMUL_LOHI) FinalOpc = ARMISD::SMLAL;
+  else return SDValue();

This would not succeed if we start off (N == ADDE) at the ADDE?

Even if the first ADD (the one taking the lo value from the UMUL_LOHI) 
were an ADDE node (and not a ADDC node) than replacing the diamond with 
UMLAL would semantically be wrong as the resulting UMLAL instruction 
would not add in the carry coming into this ADDE (as an UMLAL does not 
add a carry from a previous instruction).

Question 2:

In the following, shouldn't the HiAdd be only an ADDE? An ADDC consumes 
two values but no carry and produces two values (one normal value and 
the glue value holding the carry). An ADDE consumes three values (two 
normal values and the glue holding the carry) and produces two values 
(the result and a glue holding the carry). Because it is the HiAdd it 
must be an ADDE because it is adding in the carry value from the low add.

+  // Test the hi add is the correct add type we support
+  if (HiAdd->getOpcode() != ISD::ADDE &&
+      HiAdd->getOpcode() != ISD::ADDC) return SDValue();

Best,
Arnold

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.

On 5/3/2012 11:38 AM, Yin Ma wrote:
> Hi Anton,
>
>       Could you please take a look at this patch? It has been hanging out
> for a while.
>
> Thanks,
>
>                                Yin
>
> -----Original Message-----
> From: Anton Korobeynikov [mailto:anton at korobeynikov.info]
> Sent: Tuesday, April 17, 2012 11:41 PM
> To: Evan Cheng
> Cc: Yin Ma; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] LLVM patch to implement UMLAL/SMLAL Instructions for ARM Architecture.
>
>> Thanks. I think the patch is correct now. But I would very much
>> appreciate another pair of eyes on it.
> I will review, but it will take a while...
>
> --
> With best regards, Anton Korobeynikov
> Faculty of Mathematics and Mechanics, Saint Petersburg State University
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits