[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