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

[Evan Cheng]

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
>> 
>