[llvm-commits] LLVM patch to implement UMLAL/SMLAL Instructions for ARM Architecture.
Evan Cheng
evan.cheng at apple.com
Sat Apr 14 11:50:59 PDT 2012
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120414/eedf53fe/attachment.html>
More information about the llvm-commits
mailing list