This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Disable register stackification and coloring when not optimizing
ClosedPublic

Authored by dschuff on Feb 16 2016, 10:33 PM.

Details

Summary

These passes are optimizations, and should be disabled when not
optimizing.
Also remove the command line flag for disabling register coloring;
running llc with -O0 should now be useful for debugging, so it's not
necessary.

Diff Detail

Repository
rL LLVM

Event Timeline

dschuff updated this revision to Diff 48151.Feb 16 2016, 10:33 PM
dschuff retitled this revision from to [WebAssembly] Disable register stackification and coloring when not optimizing.
dschuff updated this object.
dschuff added reviewers: jfb, sunfish.
dschuff added a subscriber: llvm-commits.

Arguably we should make some (a lot?) of our tests run without optimization since the stackifier is orthogonal to what most of them test and it munges the output in unrelated ways and will cause churn when the stackification code changes. That doesn't seem as common in backend tests as maybe it should be.

So it turns out that getOptLevel() isn't even hooked up unless you register an MCCodeGenInfo for the target. So I added that.

dschuff updated this revision to Diff 48153.Feb 16 2016, 11:03 PM
  • Hook up MCCodeGenInfo
dschuff updated this revision to Diff 48157.Feb 16 2016, 11:18 PM
  • accept Static as well as Default reloc model
dschuff updated this revision to Diff 48159.Feb 17 2016, 12:15 AM
  • make it report_fatal_error instead of assert
sunfish edited edge metadata.Feb 17 2016, 5:06 AM

I assume the purpose here is debugging compiler output, however -O0 is generally meant for debugging source code. For example, LLVM's normal CodeGen path does do (limited) register allocation at -O0. User variables are generally stored in allocas, so they aren't affected by register allocation. Consequently, disabling register stackifying and coloring at -O0 isn't obviously the right thing to do.

Some of our tests do use -disable-block-placement, for example, to reduce uninteresting optimizer churn when it's a problem. Using -disable-wasm-register-coloring seems like it would be useful in some cases as well. A flag to disable register stackifying could be similarly useful.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.cpp
48 ↗(On Diff #48159)

I agree with being conservative for such things in general, however WebAssembly code is always PIC, so we already do support all the reloc modes. Accepting all the modes is useful because user build scripts sometimes add -fPIC in target-insensitive ways, for example. I propose we unconditionally override the mode to PIC_, as Darwin does on newer architectures.

50 ↗(On Diff #48159)

WebAssembly doesn't have offset restrictions, so it technically supports the "large" model by default. I propose we map Default and JITDefault to Large, and then accept only Large (rejecting Small, Medium, and Kernel in case we want to make those mean something some day).

dschuff updated this revision to Diff 48198.Feb 17 2016, 9:05 AM
dschuff marked 2 inline comments as done.
dschuff edited edge metadata.
  • Default to PIC and force large code model

That's a good point about user code; I don't really feel strongly about it. Although, register stackification does reorder code, which does affect debugging; so maybe we'd still want to disable that. Also presumably compilation speed is the other factor. Coloring presumably isn't linear, so maybe we'd want to disable that for speed reasons.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.cpp
48 ↗(On Diff #48159)

Good point, done.

sunfish accepted this revision.Feb 17 2016, 3:18 PM
sunfish edited edge metadata.

I expect eventually we'll want to do *some* coloring and stackification at -O0, though it's a good point that stackification can reorder things in ways that make debugging harder. Perhaps at -O0 stackification can just stackify things that don't require reordering. In any case, we can work that out later, and this patch lgtm for now.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.cpp
47 ↗(On Diff #48198)

Looks good. We should handle DefaultJIT as Large too. Of course, we don't have full JIT support yet, but that's no reason for this code not to be ready for when we do :-).

This revision is now accepted and ready to land.Feb 17 2016, 3:18 PM
This revision was automatically updated to reflect the committed changes.