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

[Jacob Gravelle via llvm-commits]

jgravelle-google added inline comments.

================
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()) {
----------------
sunfish wrote:
> 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.
Indeed it does.

And I didn't think about SmallVector. Thanks

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

================
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)
----------------
sunfish wrote:
> 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.
Right, forgot that was a thing. Thanks

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:317
@@ -316,3 @@
-    VNInfo *InsVNI = LI.getVNInfoBefore(LIS.getInstructionIndex(*Insert));
-    if (InsVNI && DefVNI != InsVNI)
-      return false;
----------------
jgravelle-google wrote:
> sunfish wrote:
> > 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.
> Right, forgot that was a thing. Thanks
Exactly.

And, nice catch on the LIS.

================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:341
@@ +340,3 @@
+      for (const MachineOperand &OpMO : I->operands())
+        if (OpMO.isReg() && OpMO.isDef() && OpMO.getReg() == MO->getReg())
+          return false;
----------------
Looking at this again, I could reorder this to only loop over the mutable operands if the instruction's operands are defs, which would be slightly more performant. I'll stick with this because it's more readable.


https://reviews.llvm.org/D24942