This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Adding 64-bit versions of __stack_pointer and other globals
ClosedPublic

Authored by aardappel on Jun 18 2020, 3:29 PM.

Details

Summary

We have 6 globals, all of which except for __table_base are 64-bit under wasm64.

Diff Detail

Event Timeline

aardappel created this revision.Jun 18 2020, 3:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 18 2020, 3:29 PM
aardappel updated this revision to Diff 271859.Jun 18 2020, 3:31 PM

This is a first iteration, probably needs more tests :) I was thinking of forking userstack.ll since it has most __stack_pointer tests, but I am not sure if it's that useful. Needs a test that uses the new wasm-ld flag. Opinions welcome.

Yeah I think a 64-bit version of userstack.ll makes sense. (a fork or a second set of CHECKs, whatever you think would be cleaner). There's also stack-alignment.ll which covers dynamic stack adjustment.

lld/wasm/Driver.cpp
385

any particular reason this shouldn't use the more conventional -m32/m64?
edit: nevermind, this is lld not clang. I'll defer to Sam's opinion on lld flags.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
84–86

should __table_base stay as i32?

aardappel marked 2 inline comments as done.Jun 22 2020, 9:25 AM

I'll likely fork userstack.ll since the majority of lines need changes.

lld/wasm/Driver.cpp
385

Yup, @sbc100 suggested this.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
84–86

I'd think so, right? since it refers to table indices, not memory

sbc100 accepted this revision.Jun 22 2020, 10:04 AM

Great!

clang/lib/Driver/ToolChains/WebAssembly.cpp
68

All the other drivers seem to do CmdArgs.push_back("-m"); .. CmdArgs.push_back("xxx");

I would expect these to be two distinct argument.. in fact I'm not sure how it works with once arg that contains a space like this.

lld/wasm/Driver.cpp
385

Yeah it looks like gnu ld and lld elf always accept -m <arch> from clang.

390

It looks like lld ELF inherits the GNU ld terminology and calls this argument "emulation".

      -m emulation
           Emulate the emulation linker.  You can list the available emulations with the
           --verbose or -V options.

           If the -m option is not used, the emulation is taken from the "LDEMULATION"
           environment variable, if that is defined.

           Otherwise, the default emulation depends upon how the linker was configured.
`

Pretty odd choice of name I think and we probably are ok no to copy that here.

But how about I think I prefer the term "architecture" to target. Seems a little more specific.

How about: error("invalid target architecture: " + s);

lld/wasm/InputChunks.cpp
338

Just use normal variable names here? opcode_const and opcode_add?

This revision is now accepted and ready to land.Jun 22 2020, 10:04 AM

So the code LGTM, were you going to add to usertest.ll in this CL?

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
84–86

Oh, right; I'd misread this as setting it just for table_base but I had it backwards; it's exempting table_base. So yeah this is right.

@dschuff still working on it, it uncovered some issues (as it should :)

aardappel marked 7 inline comments as done.Jun 25 2020, 1:38 PM
aardappel updated this revision to Diff 273503.Jun 25 2020, 1:41 PM
  • Fixed ISEL for FrameIndex
  • Fixed 64-bit conditions in branches (thanks @aheejin!)
  • Made the FrameIndex generation code in WebAssemblyRegisterInfo work.
  • Made userstack.ll and stack-alignment.ll pass in wasm64.
  • Code review fixes.
sbc100 accepted this revision.Jun 25 2020, 2:56 PM

Linker changes still lgtm % comments

lld/wasm/InputChunks.cpp
338

Normal variable names?

363

Again I think you appetite for the use of auto is exceeding that of the llvm coding style... I tend to stick to cases where the type is spelled out later in the same line.

dschuff accepted this revision.Jun 25 2020, 3:44 PM

Compiler changes LGTM

llvm/test/CodeGen/WebAssembly/stack-alignment.ll
1–2

that -D flag is really nice.

aardappel marked 3 inline comments as done.Jun 25 2020, 3:45 PM
aardappel updated this revision to Diff 273539.Jun 25 2020, 3:48 PM

types & variables in InputChunks.cpp

This revision was automatically updated to reflect the committed changes.