[llvm-commits] Request for Review - avoid register pressure with read/modify/write instructions

[Evan Cheng]

Patch looks good. Please merge the tests into dec-eflags-lower.ll. We should rename dec-eflags-lower.ll to something like rmw-eflags.ll.

Evan

On Mar 28, 2012, at 11:12 AM, Joel Jones <joel_k_jones at apple.com> wrote:

> This is a code change to add support for changing instruction sequences of the form:
> 
>   load
>   inc/dec of 8/16/32/64 bits
>   store
> 
> into the appropriate X86 inc/dec through memory instruction:
> 
>   inc[qlwb] / dec[qlwb]
> 
> The change is in lib/Target/X86/X86ISelDAGToDAG.cpp, an svn diff of which is attached.
> Also attached is a test case for test/CodeGen/X86. The checks that were in 
> X86DAGToDAGISel::Select(SDNode *Node)>>ISD::STORE have been extracted
> to isLoadIncOrDecStore and reworked to use the better
> named wrappers for getOperand(unsigned) and replaced Chain.getNode()
> with LoadNode.  I have also expanded the comments.  
> 
> To address the reviewer's comments:
> 
> With the expanded comments,  I think the broken down checks are more appropriate; 
> q.v. http://llvm.org/docs/CodingStandards.html#hl_earlyexit
> 
> I also prefer NOT/OR/== to AND/!= when all the left operands of the comparison operators 
> are the same, as I think it more clearly expresses the intent of "if the value being examined 
> isn't A or B, then do something"
> 
> Joel
> <X86ISelDAGToDAG.cpp.diff>
> <readModWrite.ll>
> 
> 
> On Mar 27, 2012, at 6:28 PM, Evan Cheng wrote:
> 
>> More nitpicks:
>> 
>> +  if (Undef->getOpcode() != ISD::UNDEF) return false;
>> +  if (Chain->getOpcode() != ISD::LOAD) return false;
>> +  if (!(Opc == X86ISD::DEC || Opc == X86ISD::INC)) return false;
>> +  if (StoredVal.getResNo() != 0) return false;
>> +  if (!StoredVal.getNode()->hasNUsesOfValue(1, 0)) return false;
>> +  if (!Chain.getNode()->hasNUsesOfValue(1, 0)) return false;
>> +  if (StoredVal->getOperand(0).getNode() != Chain.getNode()) return false;
>> 
>> There is no good reason to separate each condition into a separate check. I would, for example, combine the first two. I would also combine the StoredVal checks.
>> 
>> +  if (!(Opc == X86ISD::DEC || Opc == X86ISD::INC)) return false;
>> Why not
>> +  if (Opc != X86ISD::DEC && Opc != X86ISD::INC)
>>      return false;
>> And this probably should be the first check.
>> 
>> Please also include test cases so the reviewer can commit the patch for you once it passes inspection.
>> 
>> Evan
>> 
>> On Mar 27, 2012, at 4:05 PM, Joel Jones wrote:
>> 
>>> I have made the changes as requested and have attached the diff.
>>> 
>>> Joel
>>> <X86OISelDAGToDAG.cpp.diff>
>>> 
>>> On Mar 27, 2012, at 10:58 AM, Evan Cheng wrote:
>>> 
>>>> Comments:
>>>> 
>>>> I know some of the issues were there before your patch. But please correct them while you are at it.
>>>> 
>>>> Can you add some comments to isLdIncOrDecSt? There should be at least top level comments. Also I think there is a bug in this. Please check if the load is a non-indexed, non-extending load (using isNormalLoad()). Similarly, please  make sure the store is not a truncating store.
>>>> 
>>>> int memSize = StoreNode->getMemOperand()->getSize();
>>>> Please do not depend on memoperand for this. LSBaseSDNode has MemVT field. Or better yet, just use Load's ValueType. So replace
>>>> 
>>>> +    if (!(memSize == 8 || memSize == 4 || memSize == 2 || memSize == 1)) 
>>>> +        return false;
>>>> 
>>>> with something like:
>>>> if (LdVT != MVT::i8 && LdVT != MVT::i16 && LdVT != MVT::i32 && LdVT != MVT::i64)
>>>> return false;
>>>> 
>>>> newIncDecOpc() has some formatting issues. Indentation width should be 2.
>>>> 
>>>> One more nitpick. The names for these functions look weird. How about isLoadIncDecStore? getFusedLdStOpcode?
>>>> 
>>>> Evan
>>>> 
>>>> On Mar 26, 2012, at 6:01 PM, Joel Jones <joel_k_jones at apple.com> wrote:
>>>> 
>>>>> This is a code change to add support for changing instruction sequences of the form:
>>>>> load
>>>>> inc/dec of 8/16/32/64 bits
>>>>> store
>>>>> into the appropriate X86 inc/dec instruction:
>>>>> inc[qlwb]/dec[qlwb]
>>>>> 
>>>>> The change is in lib/Target/X86/X86ISelDAGToDAG.cpp, an svn diff of which is attached.
>>>>> Also attached is a test case.
>>>>> 
>>>>> Joel Jones
>>>>> 
>>>>> <X86OISelDAGToDAG.cpp.diff><readModWrite.ll>_______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120328/9d1d04c2/attachment.html>