[llvm-commits] LLVM patch to implement UMLAL/SMLAL Instructions for ARM Architecture.
Yin Ma
yinma at codeaurora.org
Tue Apr 17 11:29:14 PDT 2012
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>
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
<mailto:llvm-commits at cs.uiuc.edu> llvm-commits at cs.uiuc.edu
<http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
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/20120417/ff08b70e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llmlal.diff
Type: application/octet-stream
Size: 15038 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120417/ff08b70e/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: longMAC.ll
Type: application/octet-stream
Size: 1075 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120417/ff08b70e/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: longMACt.ll
Type: application/octet-stream
Size: 1092 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120417/ff08b70e/attachment-0002.obj>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: report.simple.txt
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120417/ff08b70e/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: result.txt
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120417/ff08b70e/attachment-0001.txt>
More information about the llvm-commits
mailing list