[llvm-commits] [PATCH]IDIV->DIVB Atom Optimization

[Nowicki, Tyler]

Thanks for the reply.

Lower is too late because DIV gets expanded into 2 MOV, a CDQ or CWD, the DIV or IDIV, and one or two more MOVs during instruction selection on DAG conversion in X86ISelDAGToDAG. Adding this optimization after would require a custom inserter to search and remove these instructions, and generate new BBs. This is complicated by O2 optimization which may remove MOVs when possible, by the scheduler on Atom which changes the order of instructions to avoid stalls, and by which form of the DIV instruction is used. Also, depending on the spills and neighboring instructions it is difficult to enumerate all the possible ways this optimization can be carried out.
 
It makes sense to do the optimization as an IR pass because none of this has happened yet. Then CSE and scheduling can operate on the new BBs.

Thanks,

Tyler

> -----Original Message-----
> From: Evan Cheng [mailto:evan.cheng at apple.com]
> Sent: Tuesday, July 31, 2012 6:41 PM
> To: Nowicki, Tyler
> Cc: Eli Friedman; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH]IDIV->DIVB Atom Optimization
> 
> Hi Tyler,
> 
> This doesn't look like the right approach. I don't understand why you are
> adding a LLVM ir pass to do the expansion. Why can't this be done at x86
> instruction lowering time?
> 
> Evan
> 
> On Jul 31, 2012, at 1:25 PM, "Nowicki, Tyler" <tyler.nowicki at intel.com>
> wrote:
> 
> > Thanks for the review. Good catch on the missing check, I've also made use
> of DenseMap as suggested. Test cases omitted from the previously patch are
> in place again and a test case for the missing check you found has been
> added.
> >
> > Please review my corrections.
> >
> > Tyler Nowicki
> > Intel
> >
> >> -----Original Message-----
> >> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> >> Sent: Friday, July 27, 2012 2:17 PM
> >> To: Nowicki, Tyler
> >> Cc: Sean Silva; llvm-commits at cs.uiuc.edu
> >> Subject: Re: [llvm-commits] [PATCH]IDIV->DIVB Atom Optimization
> >>
> >> On Fri, Jul 27, 2012 at 8:35 AM, Nowicki, Tyler
> >> <tyler.nowicki at intel.com>
> >> wrote:
> >>> This patch has not yet been reviewed. Could someone take a look at it?
> >>
> >> +    typedef std::pair<Value *, Value *> DivBOperands; //Dividend
> >> Value, Divisor Value
> >> +    typedef std::pair<PHINode *, PHINode *> DivBPhiNodes; //Quotient
> >> PHI, Remainder Phi
> >> +    typedef std::map<DivBOperands, DivBPhiNodes> DivBCacheTy;
> >>
> >> llvm::DenseMap?
> >>
> >> +  // Replace operation value with previously generated phi node
> >> + DivBPhiNodes Value = CacheI->second;  if (UseDivOp) {
> >> +    // Replace all uses of div instruction with quotient phi node
> >> +    J->replaceAllUsesWith(Value.first);
> >> +  } else {
> >> +    // Replace all uses of rem instruction with remainder phi node
> >> +    J->replaceAllUsesWith(Value.second);
> >> +  }
> >>
> >> Looks like there's a missing check for UseSignedOp here.
> >>
> >> The patch doesn't appear to include any testcases.
> >>
> >> -Eli
> > <idivtodivb-
> svn.patch>_______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits