commit ff40aaad06a6603da4ecaa4dc0119bcf6e50d1e0
author Cherry Zhang <cherryyz@google.com> Tue Dec 18 10:50:15 2018 -0500
committer Cherry Zhang <cherryyz@google.com> Tue Dec 18 16:09:02 2018 +0000
tree fbbba6f8d669aefd0ba4a5994e348102fb48f136
parent fc71875a4327d7ad045514e2aab8414462336023

passes: handle degenerate Phis in stack maps

A degenerate Phi of null pointer is recorded in the stack map as
constant 0. Previously we just ignored constant 0, since it
cannot be a valid pointer bitmap. However, with a type that is
larger than 32*8 bytes, constant 0 may still appear as part of
the multiword bitmap, so it is problematic to just ignore it.
Instead, filter degenerate Phis and don't record them in the
live set.

Change-Id: I2e39393c8eea8d5d1c515445b63d969796591524
Reviewed-on: https://go-review.googlesource.com/c/154738
Reviewed-by: Than McIntosh <thanm@google.com>

passes/GC.cpp

diff --git a/passes/GC.cpp b/passes/GC.cpp
index 21c8b1a..c754c50 100644
--- a/passes/GC.cpp
+++ b/passes/GC.cpp
@@ -79,11 +79,8 @@
 
       // If a slot is following a "Constant" location, it is an aggregate type
       // and that constant encodes the pointer bitmap.
-      // Constant 0 cannot be a pointer bitmap. Instead, it encodes a null pointer
-      // as live (happens with degenerated Phis).
       if (i+1 < n &&
-          CSLocs[i+1].Type == StackMaps::Location::Constant &&
-          CSLocs[i+1].Offset != 0) {
+          CSLocs[i+1].Type == StackMaps::Location::Constant) {
         // Make sure it is a local/arg slot, not a spill of in-register value.
         assert(Loc.Type == StackMaps::Location::Direct);
 

passes/GoStatepoints.cpp

diff --git a/passes/GoStatepoints.cpp b/passes/GoStatepoints.cpp
index d4ba6d0..ae506d3 100644
--- a/passes/GoStatepoints.cpp
+++ b/passes/GoStatepoints.cpp
@@ -1379,6 +1379,9 @@
   // pointer fields.
   SmallVector<Value *, 64> PtrFields;
   for (Value *V : BasePtrs) {
+    if (auto *Phi = dyn_cast<PHINode>(V))
+      if (Phi->hasConstantValue())
+        continue;
     if (isa<AllocaInst>(V) ||
         (isa<Argument>(V) && cast<Argument>(V)->hasByValAttr())) {
       // Byval argument is at a fixed frame offset. Treat it the same as alloca.
@@ -1668,11 +1671,30 @@
 }
 
 static void
-fixBadPhiOperands(Function &F, SetVector<Value *> &BadLoads) {
+fixBadPhiOperands(Function &F, SetVector<Value *> &BadLoads,
+                  SetVector<Instruction *> &ToDel) {
   for (auto *I : BadLoads) {
     I->replaceAllUsesWith(Constant::getNullValue(I->getType()));
     cast<Instruction>(I)->eraseFromParent();
   }
+
+  // The replacement above may lead to degenerate Phis, which, if live,
+  // will be encoded as constants in the stack map, which is bad
+  // (confusing with pointer bitmaps). Clean them up.
+  // Don't delete them yet -- they may be referenced in the liveness
+  // map. We'll delete them at the end.
+  bool Changed = true;
+  while (Changed) {
+    Changed = false;
+    for (Instruction &I : instructions(F))
+      if (auto *Phi = dyn_cast<PHINode>(&I))
+        if (Value *V = Phi->hasConstantValue())
+          if (!ToDel.count(Phi)) {
+            Phi->replaceAllUsesWith(V);
+            ToDel.insert(Phi);
+            Changed = true;
+          }
+  }
 }
 
 static bool insertParsePoints(Function &F, DominatorTree &DT,
@@ -1720,7 +1742,8 @@
     findBasePointers(DT, DVCache, ToUpdate[i], info);
   }
 
-  fixBadPhiOperands(F, BadLoads);
+  SetVector<Instruction *> ToDel;
+  fixBadPhiOperands(F, BadLoads, ToDel);
 
   // It is possible that non-constant live variables have a constant base.  For
   // example, a GEP with a variable offset from a global.  In this case we can
@@ -1811,6 +1834,10 @@
 
   zeroAmbiguouslyLiveSlots(F, ToZero, AddrTakenAllocas);
 
+  // Delete dead Phis.
+  for (Instruction *I : ToDel)
+    I->eraseFromParent();
+
   return !Records.empty();
 }