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

[Haicheng Wu via Phabricator via llvm-commits]

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


Repository:
  rL LLVM

https://reviews.llvm.org/D36104