[PATCH] D36104: [AArch64] Coalesce Copy Zero during instruction selection

[Geoff Berry via Phabricator via llvm-commits]

gberry added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2767
   case ISD::Constant: {
-    // Materialize zero constants as copies from WZR/XZR.  This allows
-    // the coalescer to propagate these into other instructions.
+    // If all uses of zero constants are copies to virutal regs, replace the
+    // conatants with WZR/XZR.  Otherwise, materialize zero constants as copies
----------------
haicheng wrote:
> gberry wrote:
> > haicheng wrote:
> > > gberry wrote:
> > > > haicheng wrote:
> > > > > gberry wrote:
> > > > > > Would it not make sense to replace any use of constant 0 with wzr/xzr when it is legal?
> > > > > I agree with you.  I started with replacing any constant 0 with wzr/xzr, but triggered a lot of assertions.  For example, wzr/xzr is not expected to appear as the condition of a conditional branch.  Then, I  narrowed down to CopyToReg only, but still triggered some assertions when copying wzr/xzr to another physical register.  Now, I narrow down to the most common situation, copying constant zero to a virtual reg, no assertion is trigged and performance looks good.
> > > > Yeah, that's why I said "when it is legal" :)
> > > > It made be too much of a pain to determine legality here, but what about handling cases where only some of the uses are virt reg copies (and just using xzr/wzr directly in those cases)?
> > > And I check if all the uses are copies to virtual regs and call RAUW to replace because SDUse::set() is a private method.  Do you think it is worthwhile to change the API to be public?  It does not make big difference to the performance since most of the time constant 0 has only one use.
> > No, I'm not suggesting you change SDUse (that is private for a reason).  If you want to do this experiment you would need to replace the users themselves with new nodes that read XZR/WZR directly.
> > 
> > I think either approach is okay here, but you should probably wait for someone else to approve it.
> I tried and it seems I cannot replace the users themselves with new nodes that read XZR/WZR directly.  The new node I can create here is CopyFromReg and its type is MVT::i32/i64.  The user is CopyToReg and its type is MVT::Other.  The types do not match and I cannot do the replacement.  Another potential issues is that the users can be used in the other MBB because it is lowered from a PHINode, but use_iterator seems not include this usage.
> 
> I think I would stick to the current approach which just replace #0 with XZR/WZR because it is simple and conservative but covers the most common situations.  If I miss any coalescing opportunities, they would be coalesced anyway in the later pass.
I'm not sure I fully understand the problem you're running into trying to do this forward substitution on only some uses.  If you wanted to try that you would leave this node alone and instead replace the use nodes that are vreg COPYs that are reading the COPY of XZR/WZR.


Repository:
  rL LLVM

https://reviews.llvm.org/D36104