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