[llvm-commits] [PATCH 1/6] revise/enhance atomic primitive code generation
Michael Liao
michael.liao at intel.com
Wed Sep 5 15:02:10 PDT 2012
Ping...
Thanks for review
- Michael
On Tue, 2012-08-21 at 22:02 -0700, Michael Liao wrote:
> A little revised patch again.
>
> The new change includes an optimization translating atomic-load-add on
> i16 with negative operands into lock-sub with positive operands. As i16
> is promoted to i32 in most cases, previous logic cannot convert this
> (atomic-load-add (truncate (sub 0 x))) into (lock-sub x) on i16 data
> type.
>
> Yours
> - Michael
>
> On Tue, 2012-08-21 at 16:17 -0700, Michael Liao wrote:
> > here is the revised 1st part of this series of patches. To address the
> > coverage issue, I added 4 lit tests for i8/i16/i32/i64 atomic
> > primitives. So far, I added
> > add/sub/and/or/xor/nand/max/min/umax/umin/cmpxchg/store/swap.
> > Unfortuntatelly, the current trunk can only passes the i32 one. These
> > failures are solved in the following patches in this series.
> > Yours
> > - 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
> > >
> > >
> > >
> >
> > _______________________________________________
> > 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
More information about the llvm-commits
mailing list