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

Evan Cheng evan.cheng at apple.com
Tue Apr 17 22:36:09 PDT 2012


Thanks. I think the patch is correct now. But I would very much appreciate another pair of eyes on it. 

Evan

On Apr 17, 2012, at 11:29 AM, Yin Ma <yinma at codeaurora.org> wrote:

> HI Evan,
>  
>      I have updated the source based on your latest comments.
> Thank you for reviewing.
>  
>                             Yin
>  
> From: Evan Cheng [mailto:evan.cheng at apple.com] 
> Sent: Saturday, April 14, 2012 11:51 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.
>  
> Sorry, I took a closer look and I find some issues.
>  
>  // Multiply + accumulate                                                                                                                                                                                 
> -def SMLAL : AsMul1I64<0b0000111, (outs GPR:$RdLo, GPR:$RdHi),                                                                                                                                            
> -                               (ins GPR:$Rn, GPR:$Rm), IIC_iMAC64,                                                                                                                                       
> +def SMLAL : AsMla1I64<0b0000111, (outs GPR:$RdLo, GPR:$RdHi),                                                                                                                                            
> +                               (ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi), IIC_iMAC64,                                                                                                                   
>                      "smlal", "\t$RdLo, $RdHi, $Rn, $Rm", []>,                                                                                                                                            
> -                    Requires<[IsARM, HasV6]>;                                                                                                                                                            
> -def UMLAL : AsMul1I64<0b0000101, (outs GPR:$RdLo, GPR:$RdHi),                                                                                                                                            
> -                               (ins GPR:$Rn, GPR:$Rm), IIC_iMAC64,                                                                                                                                       
> +                    RegConstraint<"$RLo = $RdLo, $RHi = $RdHi">, Requires<[IsARM, HasV6]>;                                                                                                               
> +def UMLAL : AsMla1I64<0b0000101, (outs GPR:$RdLo, GPR:$RdHi),                                                                                                                                            
> +                               (ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi), IIC_iMAC64,                                                                                                                   
>                      "umlal", "\t$RdLo, $RdHi, $Rn, $Rm", []>,                                                                                                                                            
> -                    Requires<[IsARM, HasV6]>;                                                                                                                                                            
> +                    RegConstraint<"$RLo = $RdLo, $RHi = $RdHi">, Requires<[IsARM, HasV6]>;
>  
> -let Constraints = "@earlyclobber $RdLo, at earlyclobber $RdHi" in {                                                                                                                                         
> +let Constraints = "$RLo = $RdLo,$RHi = $RdHi" in {                                                                                                                                                       
>  def SMLALv5 : ARMPseudoExpand<(outs GPR:$RdLo, GPR:$RdHi),                                                                                                                                               
> -                              (ins GPR:$Rn, GPR:$Rm, pred:$p, cc_out:$s),                                                                                                                                
> +                              (ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s),                                                                                                            
>                                4, IIC_iMAC64, [],                                                                                                                                                         
> -          (SMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, pred:$p, cc_out:$s)>,                                                                                                                           
> +          (SMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s)>,                                                                                                       
>                             Requires<[IsARM, NoV6]>;                                                                                                                                                      
>  def UMLALv5 : ARMPseudoExpand<(outs GPR:$RdLo, GPR:$RdHi),                                                                                                                                               
> -                              (ins GPR:$Rn, GPR:$Rm, pred:$p, cc_out:$s),                                                                                                                                
> -                              4, IIC_iMAC64, [],                                                                                                                                                         
> -          (UMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, pred:$p, cc_out:$s)>,                                                                                                                           
> -                           Requires<[IsARM, NoV6]>;                                                                                                                                                      
> +                               (ins GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s),                                                                                                           
> +                               4, IIC_iMAC64, [],                                                                                                                                                        
> +          (UMLAL GPR:$RdLo, GPR:$RdHi, GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi, pred:$p, cc_out:$s)>,                                                                                                      
> +                            Requires<[IsARM, NoV6]>;                                                                                                                                                     
> +}                                                                                                                                                                                                        
> + 
>  
> These are beyond 80 columns.
>  
> +  // follow the glue value to get the second add                                                                                                                                                         
> +  // don't know how much uses of the first add                                                                                                                                                           
> +  // use while check
>  
> This comment doesn't make sense. Please correct.
>  
> Evan
>  
> On Apr 12, 2012, at 11:15 AM, Evan Cheng wrote:
> 
> 
> 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>
>  
> _______________________________________________
> 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/20120417/071b4697/attachment.html>


More information about the llvm-commits mailing list