[PATCH] D24942: [WebAssembly] Make register stackification more conservative

[Dan Gohman via llvm-commits]

sunfish accepted this revision.
sunfish added a comment.
This revision is now accepted and ready to land.

lgtm.


================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:282
@@ -281,2 +281,3 @@
   // Check for register dependencies.
+  std::vector<const MachineOperand*> MutableOperands;
   for (const MachineOperand &MO : Def->operands()) {
----------------
Instead of holding MachineOperand*, this could just hold `unsigned` register numbers, which would simplify some of the code below. Also, this is a good candidate for a SmallVector, with a size of something like 4.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:308
@@ +307,3 @@
+    // a different def.
+    if (!MO.isDef() && !MRI.getUniqueVRegDef(Reg))
+      MutableOperands.push_back(&MO);
----------------
MRI.hasOneDef(Reg) would make this a little clearer than using getUniqueVRegDef.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:317
@@ -325,2 +316,3 @@
   // no additional dependencies.
-  if (!Read && !Write && !Effects && !StackPointer)
+  bool HasMutableOperands = MutableOperands.size() > 0;
+  if (!Read && !Write && !Effects && !StackPointer && !HasMutableOperands)
----------------
!MutableOperands.empty() would be a little more concise than .size() > 0.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:317
@@ -316,3 @@
-    VNInfo *InsVNI = LI.getVNInfoBefore(LIS.getInstructionIndex(*Insert));
-    if (InsVNI && DefVNI != InsVNI)
-      return false;
----------------
sunfish wrote:
> !MutableOperands.empty() would be a little more concise than .size() > 0.
Ah, the bug is the case where InsVNI is null. The register may not be live at the insert point, however it may still be clobbered between Def and the insert point.

It's unfortunate then that LiveIntervals doesn't seem to have a convenient way to ask "is this value clobbered anywhere between here and there" if it isn't already live there. But given that, this was the only use of LIS in this function, so you can remove the LIS argument now too.


https://reviews.llvm.org/D24942