<html><head><base href="x-msg://2179/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Thanks. Preliminary reviews below.<div><br></div><div>The first thing I noticed is some of your stylistic choices are different from the rest of the code:</div><div><br></div><div>if( nValue != 2 ) return SDValue();</div><div><br></div><div>LLVM doesn't use camel case for variable name. Also, there should be a space before '(', not after.</div><div><br></div><div><div>+ }else{</div><div>Should be } else {</div><div><br></div><div>Now onto the rest of the patch.</div><div><br></div><div>1.</div><div><div>+ case ARMISD::UMLAL:{</div><div>+ if (Subtarget->isThumb1Only())</div><div>+ break;</div></div><div><br></div><div>The check should not be needed, right? ARMISD::UMLAL cannot be formed for Thumb1.</div><div><br></div><div><div>+ // For UMLAL/SMLAL</div><div>+ setTargetDAGCombine(ISD::ADDC);</div><div>+ setTargetDAGCombine(ISD::ADDE);</div><div>+</div></div><div><br></div><div>Should these be guarded with !(Subtarget->isThumb1Only()?</div><div><br></div><div>2.</div><div><div>+ case ISD::ADDE: return PerformADDECCombine(N, DCI, Subtarget);</div><div>+ case ISD::ADDC: return PerformADDECCombine(N, DCI, Subtarget);</div></div><div><br></div><div>That's silly. It should be:</div><div><div>+ case ISD::ADDE:</div><div>+ case ISD::ADDC: return PerformADDECCombine(N, DCI, Subtarget);</div></div><div><br></div><div>3. Can you add more comments to AddCombineTo64bitMLAL()? Please describe what the routine is trying to match.</div><div><br></div><div>4.</div><div><div>+ // The second use must be a glue to a add</div><div>+ int nValue = N->getNumValues();</div><div>+ if( nValue != 2 ) return SDValue();</div></div><div><br></div><div>if (N->getNumValues() != 2) return SDValue();</div><div><br></div><div>The check isn't necessary. ADDE always produce two values.</div><div><br></div><div>5.</div><div><br></div><div><div>+ EVT GVT = N->getValueType(1);</div><div>+ if( VT != MVT::i32 || GVT != MVT::Glue ) return SDValue();</div><div><br></div><div>VT must be MVT::i32 after legalization, right? Are there cases where GVT might not be MVT::Glue?</div><div><br></div><div>+</div><div>+ // look for glue value</div><div>+ SDNode::use_iterator UI = N->use_begin();</div><div>+ SDNode::use_iterator UE = N->use_end();</div><div>+ SDNode* HiAdd = NULL;</div><div>+ while( UI != UE ){</div><div>+ SDUse& Nuse = UI.getUse();</div><div>+ if( Nuse.getResNo() == 1 ){</div><div>+ HiAdd = Nuse.getUser();</div><div>+ break;</div><div>+ }</div><div>+ UI++;</div><div>+ }</div></div><div><br></div><div>Is a loop necessary? A glue value can only have a single use.</div><div><br></div><div>6. The rest of the routine has more stylistic issues. Also it's hard to understand without high level description.</div><div><br></div><div>7. </div><div><div>+/// PerformADDECCombine - Target-specific dag combine xforms for ISD::ADDE & ISD::ADDC for UMLAL.</div><div>+///</div></div><div><br></div><div>Please be aware of 80 col violation.</div><div><br></div><div><div>+ SDValue Result = AddCombineTo64bitMLAL(N, DCI, Subtarget);</div><div>+ if (Result.getNode())</div><div>+ return Result;</div><div>+</div><div>+ // If that didn't work, try again with the operands commuted.</div><div>+ return SDValue();</div><div>+}</div></div><div><br></div><div>Isn't this just?</div><div>return AddCombineTo64bitMLAL(N, DCI, Subtarget);</div><div><br></div><div>This routine seems unnecessary.</div><div><br></div><div>The comment indicates it should try again with operands commuted. That should be done in AddCombineTo64bitMLAL, no?</div><div><br></div><div>8.</div><div>There are a lot of failures in report.simple.txt. Why is that?<div><br><div>Evan</div><div><br><div><div>On Mar 8, 2012, at 4:15 PM, Yin Ma <<a href="mailto:yinma@codeaurora.org">yinma@codeaurora.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div class="WordSection1" style="page: WordSection1; "><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">The current definition of UMLAL/SMLAL in LLVM for ARM is not used and the<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">definition is not correct because the instruction reads the four values<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">as the input values instead of two values defined in the .td file.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">I have created a bugzilla entry regarding to this issue:<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><a href="http://llvm.org/bugs/show_bug.cgi?id=12213" style="color: purple; text-decoration: underline; ">http://llvm.org/bugs/show_bug.cgi?id=12213</a><o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">I am proposing a patch not only fixed the definition but also added the corresponding<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">generation algorithm on DAG. This algorithm only operates on ARM backend. It identifies the<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">opportunity of conversions during DAG process.<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">llmla.diff is the code change<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">longMAC.ll is the test case for ARM<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">longMACt.ll is the test case for Thumb2<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">report.simple.txt is the result from test-suites<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">result.txt is the result from test<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Please give a review. Thanks,<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "> Yin<o:p></o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-left: 0in; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div></div><span><llmlal.diff></span><span><longMAC.ll></span><span><longMACt.ll></span><span><report.simple.txt></span><span><result.txt></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="color: purple; text-decoration: underline; ">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></div></div></div></div></body></html>