Page MenuHomePhabricator

[WebAssembly] Implement prolog/epilog insertion and FrameIndex elimination
ClosedPublic

Authored by dschuff on Dec 8 2015, 11:37 AM.

Details

Summary

Use the SP32 physical register as the base for FrameIndex lowering. Update it and the __stack_pointer global var in the prolog and epilog. Extend the mapping of virtual registers to wasm locals to include the physical registers.

Rather than modify the target-independent PrologEpilogInserter (which asserts that there are no virtual registers left) Include a slightly-modified copy for Wasm that does not have this assertion and only clears the virtual registers if scavenging was needed (which of course it isn't for wasm).

Diff Detail

Repository
rL LLVM

Event Timeline

dschuff updated this revision to Diff 42205.Dec 8 2015, 11:37 AM
dschuff retitled this revision from to [WebAssembly] WIP for SP lowering..
dschuff updated this object.
dschuff added reviewers: sunfish, jfb.
dschuff added a subscriber: llvm-commits.

Nevermind, this won't work. I'll just make it an MCSymbol in MI too. hold off on the review

dschuff updated this revision to Diff 42310.Dec 9 2015, 9:22 AM
  • Use SP32 as phys reg for FI lowering and update printer to handle it. Make prolog/epilog update SP global var
dschuff updated this object.Dec 9 2015, 9:24 AM

In this version the SP32 physical register is used as the base for FrameIndex lowering. It and the @SP global var are updated in the prolog and epilog. An alternative would be just to use an arbitrary virtual register (which would be have to be stashed by the prolog e.g. in WebAssemblyRegisterInfo) but using SP seems easier to understand in the IR and not really any different in the final output.

sunfish edited edge metadata.Dec 9 2015, 1:00 PM

I'm still reviewing this, but here are a few initial comments.

lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
74 ↗(On Diff #42310)

We also need a prologue when MFI->adjustsStack(). Can you assert that doesn't happen for now?

84 ↗(On Diff #42310)

Instead of using getNamedGlobal on the Module, which requires a declaration in LLVM IR, we can use MF.createExternalSymbolName (and addExternalSymbol below), which creates an ExternalSymbol, which is a very lightweight thing that doesn't require any up-front setup.

Also, "SP" is in the C/C++ user namespace. What do you think about "__stack_pointer"?

110 ↗(On Diff #42310)

Can you assert MFI->getCalleeSavedInfo().empty(), just to double-check ourselves?

145 ↗(On Diff #42310)

This code doesn't yet set up FP; can you assert !hasFP(MF) for now?

Can you add TODO comments for:

  • Do ".setMIFlag(MachineInstr::FrameSetup)" on emitted instructions
  • Emit TargetOpcode::CFI_INSTRUCTION instructions
dschuff updated this revision to Diff 42333.Dec 9 2015, 1:34 PM
dschuff marked 3 inline comments as done.
dschuff edited edge metadata.
  • Use SP32 as phys reg for FI lowering and update printer to handle it. Make prolog/epilog update SP global var
  • Address first round of comments
lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
84 ↗(On Diff #42310)

Done. One consequence is that the MachinePointerInfo no longer has the reference to the stack pointer, but that's probably OK.

145 ↗(On Diff #42310)

hasFP() currently includes MFI->hasCalls(), which seems wrong, at least for wasm. I took that out.

Added the former TODO to EmitPrologue and the latter to the top with the other TODOs.

dschuff updated this object.Dec 9 2015, 1:36 PM
sunfish accepted this revision.Dec 9 2015, 2:04 PM
sunfish edited edge metadata.

Looks good.

lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
84 ↗(On Diff #42333)

Ah. This might be a job for PseudoSourceValue::ExternalSymbolCallEntry then. In fact, it looks like it would work to just use that right now; the only odd part is the "Call" in the name, which may be an artifact of the fact that the only thing using it right now is doing GOT loads for calls. Perhaps we could rename/generalize it though. In any case, this isn't urgent, so perhaps just add a comment mentioning this.

151 ↗(On Diff #42333)

Sounds good.

test/CodeGen/WebAssembly/userstack.ll
8 ↗(On Diff #42333)

With ExternalSymbol to get __stack_pointer, we no longer need to declare it at the LLVM IR level.

13 ↗(On Diff #42333)

Is this function still needed now that we're using an ExternalSymbol for __stack_pointer?

This revision is now accepted and ready to land.Dec 9 2015, 2:04 PM
dschuff updated this revision to Diff 42339.Dec 9 2015, 2:15 PM
dschuff marked 2 inline comments as done.
dschuff edited edge metadata.
  • Address final comments
test/CodeGen/WebAssembly/userstack.ll
13 ↗(On Diff #42333)

No, in fact it was never actually needed, just leftover.

dschuff retitled this revision from [WebAssembly] WIP for SP lowering. to [WebAssembly] Implement prolog/epilog insertion and FrameIndex elimination.Dec 9 2015, 2:17 PM
dschuff updated this object.
dschuff updated this revision to Diff 42589.Dec 11 2015, 2:42 PM
  • Remove changes to target-independent PEI, add WasmPEI
dschuff updated this object.Dec 11 2015, 2:45 PM

Based on the current state of the discussion, either supporting virtual registers after RA or switching to physical registers will be a longer-term proposition. So I've included a modified PEI pass for WebAssembly and reverted the changes to the target-independent one (similarly to NVPTX) to allow us to make progress for now.

lgtm (with the new changes).

lib/Target/WebAssembly/WebAssembly.h
42 ↗(On Diff #42589)

It isn't stated so, but the passes above are ordered in the order that they run (except for Relooper, which is not currently run). Would you mind moving the PEI pass up just below RegColoring?

lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
154 ↗(On Diff #42589)

Nit: unnecessary blank line here.

lib/Target/WebAssembly/WebAssemblyPEI.cpp
185 ↗(On Diff #42589)

This is one of the changes to the generic PEI, right? Could you add a comment?

dschuff updated this revision to Diff 42596.Dec 11 2015, 3:46 PM
dschuff marked 3 inline comments as done.
  • Address comments on updated CL
lib/Target/WebAssembly/WebAssemblyPEI.cpp
233 ↗(On Diff #42589)

Also added comment here, the other "real" diff from generic PEI

This revision was automatically updated to reflect the committed changes.