[llvm-commits] Request for Review - avoid register pressure with read/modify/write instructions
Joel Jones
joel_k_jones at apple.com
Wed Mar 28 11:12:49 PDT 2012
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>
More information about the llvm-commits
mailing list