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

[Evan Cheng]

LGTM. Can someone commit this for you?

Evan

On Apr 2, 2012, at 10:22 AM, Yin Ma wrote:

> Hi,
>  
> I have updated the code based on your comments. I have put more comments
> on the code, especially for some conditional statements I cannot remove. Please
> review again.
>  
> Llmlal.diff is the code diff for the updated version
> Reports.simple.txt and result.txt are the new test results
>  
> Thanks,
>  
>                            Yin
>  
>  
> From: Evan Cheng [mailto:evan.cheng at apple.com] 
> Sent: Thursday, March 15, 2012 11:07 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.
>  
>  
> On Mar 15, 2012, at 10:17 AM, Yin Ma wrote:
> 
> 
> 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.
>  
> Ok. Please try to get a clean run. Otherwise it's not particularly useful to include as part of the patch review.
>  
> Evan
> 
> 
>  
> 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
>  
> 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
>  
>  
> <llmlal.diff><longMAC.ll><longMACt.ll><report.simple.txt><result.txt>

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