<div>Just to be clear, you're explicitly not changing any functionality here, correct? Can you try introducing a few bugs into the code and make sure the tests start failing? I worry about our coverage here.</div><div>
<br></div><div>When glancing at the entire series of patches, I wanted to mention a few particular style points. Many of these were here before you got there, but I would appreciate switching to a style consistent with the new coding conventions:</div>
<div><br></div><div>-  bool isSomethingWrong;  -->  bool IsSomethingWrong;</div><div>-  void BuildSomeThing(...)   -->  void buildSomeThing(...)<br></div><div>-  Always use early-exit when you can, and don't use 'else' or 'break' after a return.</div>
<div><br></div><div>Note that the latter only applies for static functions or functions that aren't grouped with alarge interface still using the old naming pattern of CamelCase. We don't want that level of inconsistency, but for new functions that are static and not associated with others, camelCase is preferred.</div>
<div><br></div><div><br></div><div>Detailed comments on this first patch in-line:<br></div><div><br></div><div><div>+  </div><div>+  bool isUnOp = false, isCN = false;</div><div>   ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Val);</div>
<div>   if (CN && (int32_t)CN->getSExtValue() == CN->getSExtValue()) {</div><div>     isCN = true;</div><div>-    Val = CurDAG->getTargetConstant(CN->getSExtValue(), NVT);</div><div>+    int64_t CNVal = CN->getSExtValue();</div>
<div>+    if (Op == ADD) {</div><div>+      if ((CNVal == 1) || (CNVal == -1)) {</div><div>+        Op = (CNVal == 1) ? INC : DEC;</div><div>+        isUnOp = true;</div><div>+        // Reset isCN as opcode is changed to INC/DEC, there's no more constant</div>
<div>+        // operand.</div><div>+        isCN = false;</div><div>+      } else {</div><div>+        if (CNVal < 0) {</div><div>+          Op = SUB;</div><div>+          CNVal = -CNVal;</div><div>+        }</div><div>
+        Val = CurDAG->getTargetConstant(CNVal, NVT);</div><div>+      }</div><div>+    } else</div><div>+      Val = CurDAG->getTargetConstant(CNVal, NVT);</div><div>+  } else if (Op == ADD && Val.hasOneUse() &&</div>
<div>+             Val.getOpcode() == ISD::SUB &&</div><div>+             X86::isZeroNode(Val.getOperand(0))) {</div><div>+    Op = SUB;</div><div>+    Val = Val.getOperand(1);</div><div>   }</div></div><div><br></div>
<div>This nesting is really rough to read. Can you extract this into a helper function so you can use early exit f.ex. with the unary path?</div><div><br></div><div>Also, the last else if clause could really use a comment to explain what it is doing. (I think I understand it now, but that's on the 3rd or 4th reading...)</div>
<div><br></div><div><br></div><div><div>-  SDValue Ops[] = { Tmp0, Tmp1, Tmp2, Tmp3, Tmp4, Val, Chain };</div><div>-  SDValue Ret = SDValue(CurDAG->getMachineNode(Opc, dl, MVT::Other, Ops, 7), 0);</div><div>-  cast<MachineSDNode>(Ret)->setMemRefs(MemOp, MemOp + 1);</div>
<div>-  SDValue RetVals[] = { Undef, Ret };</div><div>-  return CurDAG->getMergeValues(RetVals, 2, dl).getNode();</div><div>+  if (isUnOp) {</div><div>+    SDValue Ops[] = { Tmp0, Tmp1, Tmp2, Tmp3, Tmp4, Chain };</div>
<div>+    SDValue Ret = SDValue(CurDAG->getMachineNode(Opc, dl, MVT::Other, Ops, 6), 0);</div><div>+    cast<MachineSDNode>(Ret)->setMemRefs(MemOp, MemOp + 1);</div><div>+    SDValue RetVals[] = { Undef, Ret };</div>
<div>+    return CurDAG->getMergeValues(RetVals, 2, dl).getNode();</div><div>+  } else {</div><div><br></div><div>No else after a return.</div><div><br></div><div>+    SDValue Ops[] = { Tmp0, Tmp1, Tmp2, Tmp3, Tmp4, Val, Chain };</div>
<div>+    SDValue Ret = SDValue(CurDAG->getMachineNode(Opc, dl, MVT::Other, Ops, 7), 0);</div><div><br></div><div>Instead of hard-coding 7, you can use the array length routine in LLVM's support library...</div><div>
<br></div><div>+    cast<MachineSDNode>(Ret)->setMemRefs(MemOp, MemOp + 1);</div><div>+    SDValue RetVals[] = { Undef, Ret };</div><div>+    return CurDAG->getMergeValues(RetVals, 2, dl).getNode();</div><div>
<br></div><div>These last three lines are shared between the unary and binary sides. Why not just declare Ret above, clobber it in the two ifs, and then share thecode to build the return?</div><div><br></div><div>+  }</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Aug 18, 2012 at 9:12 PM, Michael Liao <span dir="ltr"><<a href="mailto:michael.liao@intel.com" target="_blank">michael.liao@intel.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Rebased again onto r162180 and resolved all conflicts with recent<br>
changes. Really appreciate your time to review this patch.<br>
<br>
Yours<br>
<span class="HOEnZb"><font color="#888888">- Michael<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Wed, 2012-08-15 at 12:40 -0700, Michael Liao wrote:<br>
> To help review the patches, I rebased them to the latest trunk (r161979)<br>
> and re-submitted again.<br>
><br>
> Here is the 1st part.<br>
><br>
> Yours<br>
> - Michael<br>
><br>
> On Fri, 2012-08-10 at 14:09 -0700, Eric Christopher wrote:<br>
> > I'll try to get to this in the next couple of days. :)<br>
> ><br>
> > -eric<br>
> ><br>
> > On Aug 10, 2012, at 1:53 PM, "Liao, Michael" <<a href="mailto:michael.liao@intel.com">michael.liao@intel.com</a>> wrote:<br>
> ><br>
> > > Ping again for code review.<br>
> > ><br>
> > > Yours<br>
> > > - Michael<br>
> > ><br>
> > > -----Original Message-----<br>
> > > From: <a href="mailto:llvm-commits-bounces@cs.uiuc.edu">llvm-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:llvm-commits-bounces@cs.uiuc.edu">llvm-commits-bounces@cs.uiuc.edu</a>] On Behalf Of Michael Liao<br>

> > > Sent: Monday, July 30, 2012 5:22 PM<br>
> > > To: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > > Subject: [llvm-commits] [PATCH 1/6] revise/enhance atomic primitive code generation<br>
> > ><br>
> > > Hi<br>
> > ><br>
> > > Here's the 1st patch from a series of patch revising/enhancing the current atomic primitive code generation support in X86 backend. This patch unify the logic for load-add code selection and other load-arith operations.<br>

> > ><br>
> > > Please review them and commit if they're OK.<br>
> > ><br>
> > > Yours<br>
> > > - Michael<br>
> > ><br>
> > ><br>
> > > _______________________________________________<br>
> > > llvm-commits mailing list<br>
> > > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> ><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>