[llvm-commits] Patch to implement UMLAL/SMLAL Instructions for the ARM Architecture
Jiangning Liu
liujiangning1 at gmail.com
Sun Aug 5 13:58:31 PDT 2012
+class AsMla1I64<bits<7> opcod, dag oops, dag iops, InstrItinClass itin,
+ string opc, string asm, list<dag> pattern>
+ : AsMul1I<opcod, oops, iops, itin, opc, asm, pattern> {
+ bits<4> RdLo;
+ bits<4> RdHi;
+ bits<4> Rm;
+ bits<4> Rn;
+ bits<4> RLo;
+ bits<4> RHi;
+ let Inst{19-16} = RdHi;
+ let Inst{15-12} = RdLo;
+ let Inst{11-8} = Rm;
+ let Inst{3-0} = Rn;
+}
Hi Arnold,
Here it seems that RLo and RHi are useless in this class, right? If it's no, could you please point me where they are being referenced?
Thanks,
-Jiangning
在 2012-8-2，下午10:57，Arnold Schwaighofer <arnolds at codeaurora.org> 写道：
> Patch to implement UMLAL/SMLAL Instructions for the ARM Architecture
>
> This patch corrects the definition of umlal/smlal instructions and adds support for matching them to the ARM dag combiner.
>
>
>
> This patch got lost on my end. Sorry.
>
> Is it okay to commit?
>
> Prior discussion:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120507/142351.html
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120416/141153.html
>
> Bug:
> http://llvm.org/bugs/show_bug.cgi?id=12213
>
>> I understand Anton has not gotten back to you. That's fine, I don't think
>> you need to wait for him. However, it looks like Arnold has some concerns
>> about the patch? Please address his concerns first.
>>
>> Once you have addressed Arnold's concerns, he can commit it for you.
>> Arnold has commit privilege.
>>
>> Evan
>>
>> On May 9, 2012, at 4:32 PM, Yin Ma wrote:
>>
>>> Hi Evan,
>>>
>>> Sorry for bothering you. I would like to know if this patch is
>>> likely to be reviewed
>>> And merged into the main trunk eventually? It has been a while after I
>>> submitted this
>>> patch to the list. We hope this change could be merged into main trunk
>>> so my team could
>>> use it in the llvm official version. But we have waited for quite a
>>> while. We like to know
>>> if this is still possible. Please take a look and give us some advice.
>>> Thank you.
>>>
>>> Sincerely,
>>>
>>> Yin
>>>
>>> From: Evan Cheng [mailto:evan.cheng at apple.com]
>>> Sent: Tuesday, April 17, 2012 10:36 PM
>>> 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. 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 didnt 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>
>>
>>
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.
> <yinmas_mla.patch>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list