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

[Yin Ma]

Hi Evan,

 

     Thank you for review this patch. I will rework on the code style based

on your advice. Then please review it again. 

     

For the question #8

8.

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

 

Those failures are not caused by my change. It is due to our current test
machine setup.

This patch didn't increase any number of failures. After reworking on the
style, I will

Look for a better machine to run unit test. The failure number will change. 

 

Thanks,

 

                 Yin 

 

 

 

From: Evan Cheng [mailto:evan.cheng at apple.com] 
Sent: Thursday, March 15, 2012 12:01 AM
To: Yin Ma
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] LLVM patch to implement UMLAL/SMLAL Instructions
for ARM Architecture.

 

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>
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
 <mailto:llvm-commits at cs.uiuc.edu> llvm-commits at cs.uiuc.edu
 <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
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/0215bb7f/attachment.html>