[llvm-commits] [PATCH 1/6] revise/enhance atomic primitive code generation
Michael Liao
michael.liao at intel.com
Tue Aug 21 22:02:59 PDT 2012
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
-------------- 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/20120821/0b58868b/attachment.bin>
More information about the llvm-commits
mailing list