[llvm-commits] Patch to implement UMLAL/SMLAL Instructions for the ARM Architecture
Arnold Schwaighofer
arnolds at codeaurora.org
Sun Aug 5 15:28:32 PDT 2012
Yes, you are right - that must have been an oversight. I'll remove them
before commiting.
Thanks for catching this.
> +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 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>
>>>
>>>
>>
>>
>> --
>> 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
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.
More information about the llvm-commits
mailing list