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.

Diff Detail

Repository
rL LLVM

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
282 ↗(On Diff #72576)

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.

308 ↗(On Diff #72576)

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

317 ↗(On Diff #72576)

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

317 ↗(On Diff #72576)

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
282 ↗(On Diff #72576)

Indeed it does.

And I didn't think about SmallVector. Thanks

308 ↗(On Diff #72576)

That it would.

317 ↗(On Diff #72576)

Right, forgot that was a thing. Thanks

317 ↗(On Diff #72576)

Exactly.

And, nice catch on the LIS.

341 ↗(On Diff #72576)

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.