This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Make register stackification more conservative
ClosedPublic

Authored by jgravelle-google on Sep 26 2016, 3:20 PM.

Details

Summary

Register stackification currently checks VNInfo for changes. Make that
more accurate by testing each intervening instruction for any other defs
to the same virtual register.

Event Timeline

jgravelle-google retitled this revision from to Make register stackification more conservative.
jgravelle-google updated this object.
jgravelle-google retitled this revision from Make register stackification more conservative to [WebAssembly] Make register stackification more conservative.Sep 27 2016, 10:05 AM
sunfish accepted this revision.Sep 28 2016, 4:05 PM
sunfish edited edge metadata.

lgtm.

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
281

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.

307

MRI.hasOneDef(Reg) would make this a little clearer than using getUniqueVRegDef.

316–317

!MutableOperands.empty() would be a little more concise than .size() > 0.

317

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.

This revision is now accepted and ready to land.Sep 28 2016, 4:05 PM
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
281

Indeed it does.

And I didn't think about SmallVector. Thanks

307

That it would.

316–317

Right, forgot that was a thing. Thanks

317

Exactly.

And, nice catch on the LIS.

340

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.

jgravelle-google edited edge metadata.
  • Code review feedback
dschuff accepted this revision.Sep 30 2016, 11:09 AM
dschuff edited edge metadata.
This revision was automatically updated to reflect the committed changes.