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

[Evan Cheng]

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