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

[Evan Cheng]

Thanks. Preliminary reviews below.

The first thing I noticed is some of your stylistic choices are different from the rest of the code:

if( nValue != 2 ) return SDValue();

LLVM doesn't use camel case for variable name. Also, there should be a space before '(', not after.

+    }else{
Should be } else {

Now onto the rest of the patch.

1.
+  case ARMISD::UMLAL:{
+    if (Subtarget->isThumb1Only())
+      break;

The check should not be needed, right? ARMISD::UMLAL cannot be formed for Thumb1.

+  // For UMLAL/SMLAL
+  setTargetDAGCombine(ISD::ADDC);
+  setTargetDAGCombine(ISD::ADDE);
+

Should these be guarded with !(Subtarget->isThumb1Only()?

2.
+  case ISD::ADDE:       return PerformADDECCombine(N, DCI, Subtarget);
+  case ISD::ADDC:       return PerformADDECCombine(N, DCI, Subtarget);

That's silly. It should be:
+  case ISD::ADDE:
+  case ISD::ADDC:       return PerformADDECCombine(N, DCI, Subtarget);

3. Can you add more comments to AddCombineTo64bitMLAL()? Please describe what the routine is trying to match.

4.
+  // The second use must be a glue to a add
+  int nValue = N->getNumValues();
+  if( nValue != 2 ) return SDValue();

if (N->getNumValues() != 2) return SDValue();

The check isn't necessary. ADDE always produce two values.

5.

+  EVT GVT = N->getValueType(1);
+  if( VT != MVT::i32 || GVT != MVT::Glue ) return SDValue();

VT must be MVT::i32 after legalization, right? Are there cases where GVT might not be MVT::Glue?

+
+  // look for glue 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++;
+  }

Is a loop necessary? A glue value can only have a single use.

6. The rest of the routine has more stylistic issues. Also it's hard to understand without high level description.

7. 
+/// PerformADDECCombine - Target-specific dag combine xforms for ISD::ADDE & ISD::ADDC for UMLAL.
+///

Please be aware of 80 col violation.

+  SDValue Result = AddCombineTo64bitMLAL(N, DCI, Subtarget);
+  if (Result.getNode())
+      return Result;
+
+  // If that didn't work, try again with the operands commuted.
+  return SDValue();
+}

Isn't this just?
return AddCombineTo64bitMLAL(N, DCI, Subtarget);

This routine seems unnecessary.

The comment indicates it should try again with operands commuted. That should be done in AddCombineTo64bitMLAL, no?

8.
There are a lot of failures in report.simple.txt. Why is that?

Evan

On Mar 8, 2012, at 4:15 PM, Yin Ma <yinma at codeaurora.org> wrote:

> The current definition of UMLAL/SMLAL in LLVM for ARM is not used and the
> definition is not correct because the instruction reads the four values
> as the input values instead of two values defined in the .td file.
>  
> I have created a bugzilla entry  regarding to this issue:
> http://llvm.org/bugs/show_bug.cgi?id=12213
>  
> I am proposing a patch not only fixed the definition but also added the corresponding
> generation algorithm on DAG. This algorithm only operates on ARM backend. It identifies the
> opportunity of conversions during DAG process.
>  
> llmla.diff is the code change
> longMAC.ll is the test case for ARM
> longMACt.ll is the test case for Thumb2
> report.simple.txt is the result from test-suites
> result.txt is the result from test
>  
> Please give a review. Thanks,
>  
>                                         Yin
>  
>  
> <llmlal.diff><longMAC.ll><longMACt.ll><report.simple.txt><result.txt>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120315/7efc7de1/attachment.html>