[llvm-commits] [PATCH 1/6] revise/enhance atomic primitive code generation

[Chandler Carruth]

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.

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:

-  bool isSomethingWrong;  -->  bool IsSomethingWrong;
-  void BuildSomeThing(...)   -->  void buildSomeThing(...)
-  Always use early-exit when you can, and don't use 'else' or 'break'
after a return.

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.


Detailed comments on this first patch in-line:

+
+  bool isUnOp = false, isCN = false;
   ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Val);
   if (CN && (int32_t)CN->getSExtValue() == CN->getSExtValue()) {
     isCN = true;
-    Val = CurDAG->getTargetConstant(CN->getSExtValue(), NVT);
+    int64_t CNVal = CN->getSExtValue();
+    if (Op == ADD) {
+      if ((CNVal == 1) || (CNVal == -1)) {
+        Op = (CNVal == 1) ? INC : DEC;
+        isUnOp = true;
+        // Reset isCN as opcode is changed to INC/DEC, there's no more
constant
+        // operand.
+        isCN = false;
+      } else {
+        if (CNVal < 0) {
+          Op = SUB;
+          CNVal = -CNVal;
+        }
+        Val = CurDAG->getTargetConstant(CNVal, NVT);
+      }
+    } else
+      Val = CurDAG->getTargetConstant(CNVal, NVT);
+  } else if (Op == ADD && Val.hasOneUse() &&
+             Val.getOpcode() == ISD::SUB &&
+             X86::isZeroNode(Val.getOperand(0))) {
+    Op = SUB;
+    Val = Val.getOperand(1);
   }

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?

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...)


-  SDValue Ops[] = { Tmp0, Tmp1, Tmp2, Tmp3, Tmp4, Val, Chain };
-  SDValue Ret = SDValue(CurDAG->getMachineNode(Opc, dl, MVT::Other, Ops,
7), 0);
-  cast<MachineSDNode>(Ret)->setMemRefs(MemOp, MemOp + 1);
-  SDValue RetVals[] = { Undef, Ret };
-  return CurDAG->getMergeValues(RetVals, 2, dl).getNode();
+  if (isUnOp) {
+    SDValue Ops[] = { Tmp0, Tmp1, Tmp2, Tmp3, Tmp4, Chain };
+    SDValue Ret = SDValue(CurDAG->getMachineNode(Opc, dl, MVT::Other, Ops,
6), 0);
+    cast<MachineSDNode>(Ret)->setMemRefs(MemOp, MemOp + 1);
+    SDValue RetVals[] = { Undef, Ret };
+    return CurDAG->getMergeValues(RetVals, 2, dl).getNode();
+  } else {

No else after a return.

+    SDValue Ops[] = { Tmp0, Tmp1, Tmp2, Tmp3, Tmp4, Val, Chain };
+    SDValue Ret = SDValue(CurDAG->getMachineNode(Opc, dl, MVT::Other, Ops,
7), 0);

Instead of hard-coding 7, you can use the array length routine in LLVM's
support library...

+    cast<MachineSDNode>(Ret)->setMemRefs(MemOp, MemOp + 1);
+    SDValue RetVals[] = { Undef, Ret };
+    return CurDAG->getMergeValues(RetVals, 2, dl).getNode();

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?

+  }


On Sat, Aug 18, 2012 at 9:12 PM, Michael Liao <michael.liao at intel.com>wrote:

> Rebased again onto r162180 and resolved all conflicts with recent
> changes. Really appreciate your time to review this patch.
>
> Yours
> - Michael
>
> On Wed, 2012-08-15 at 12:40 -0700, Michael Liao wrote:
> > To help review the patches, I rebased them to the latest trunk (r161979)
> > and re-submitted again.
> >
> > Here is the 1st part.
> >
> > Yours
> > - Michael
> >
> > On Fri, 2012-08-10 at 14:09 -0700, Eric Christopher wrote:
> > > I'll try to get to this in the next couple of days. :)
> > >
> > > -eric
> > >
> > > On Aug 10, 2012, at 1:53 PM, "Liao, Michael" <michael.liao at intel.com>
> wrote:
> > >
> > > > Ping again for code review.
> > > >
> > > > Yours
> > > > - Michael
> > > >
> > > > -----Original Message-----
> > > > From: llvm-commits-bounces at cs.uiuc.edu [mailto:
> llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Michael Liao
> > > > Sent: Monday, July 30, 2012 5:22 PM
> > > > To: llvm-commits at cs.uiuc.edu
> > > > Subject: [llvm-commits] [PATCH 1/6] revise/enhance atomic primitive
> code generation
> > > >
> > > > Hi
> > > >
> > > > 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.
> > > >
> > > > Please review them and commit if they're OK.
> > > >
> > > > Yours
> > > > - Michael
> > > >
> > > >
> > > > _______________________________________________
> > > > llvm-commits mailing list
> > > > llvm-commits at cs.uiuc.edu
> > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> 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/20120821/f50502fe/attachment.html>