[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:
> > 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)?


================
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:
> > > > 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D36104