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

[Joel Jones]

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


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/9451fd22/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: X86ISelDAGToDAG.cpp.diff
Type: application/octet-stream
Size: 6531 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120328/9451fd22/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120328/9451fd22/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: readModWrite.ll
Type: application/octet-stream
Size: 3448 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120328/9451fd22/attachment-0001.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120328/9451fd22/attachment-0002.html>