[llvm-commits] [PATCH 1/6] revise/enhance atomic primitive code generation
Michael Liao
michael.liao at intel.com
Mon Sep 17 11:48:23 PDT 2012
Ping again after almost 1 month lag. The patch is rebased against the
trunk.
Thanks for your review
- Michael
On Tue, 2012-08-21 at 12:15 -0700, Chandler Carruth wrote:
> 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 --------------
A non-text attachment was scrubbed...
Name: 0001-unify-the-logic-in-SelectAtomicLoadAdd-and-SelectAto.patch
Type: text/x-patch
Size: 38898 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120917/53f1b7a7/attachment.bin>
More information about the llvm-commits
mailing list