Page MenuHomePhabricator

[WebAssembly] Skeleton WebAssembly target
AbandonedPublic

Authored by sunfish on Jun 19 2015, 7:28 AM.

Details

Reviewers
None
Summary

This patch introduces the WebAssembly target triples and a skeleton WebAssembly target in lib/Target/WebAssembly, per discussion in [0].

[0] http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-June/086890.html

Diff Detail

Event Timeline

sunfish updated this revision to Diff 28024.Jun 19 2015, 7:28 AM
sunfish retitled this revision from to [WebAssembly] Skeleton WebAssembly target.
sunfish updated this object.
sunfish edited the test plan for this revision. (Show Details)
sunfish added subscribers: jfb, Unknown Object (MLST).

I am really happy to see this!

I just tried to build it with cmake and got some errors:

WebAssemblyInstPrinter.cpp:28:10: fatal error:
'WebAssemblyGenAsmWriter.inc' file not found
WebAssemblyISelDAGToDAG.cpp:60:10: fatal error:
'WebAssemblyGenDAGISel.inc' file not found

Do code models and relocation models have any meaning in webassembly?

Is SIMD an optional feature?

Atomics? Is there a memory model? You mentioned that there is nothing
like C's undefined behavior...

It is a bit funny that webassembly is an OS. Targeting web assembly
always implies the same apis, no?

Cheers,
Rafael

sunfish updated this revision to Diff 28045.Jun 19 2015, 1:50 PM

Update to fix includes of generated include files and remove the code for code models and relocation models.

jfb added inline comments.Jun 29 2015, 2:28 PM
CODE_OWNERS.TXT
70

Could you add me as well for WebAssembly? I could do a different change too...

include/llvm/ADT/Triple.h
89

Add a mention of little-endian in both comment.

156–157

I'd rather spell the OS enum the same as its textual representation in IR (uppercase enum, lowercase string seems to be the trend). That's not a problem anymore since arch are wasm32/wasm64, so I'd rather have WASM as the OS.

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
36

report_fatal_error instead, here and below?

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp
25

PointerSize is 4 by default, we need to set it for wasm64.
CalleeSaveStackSlotSize doesn't seem relevant, so the 4 default is fine?

(a bunch of others have OK defaults)

MaxInstLength doesn't seem relevant? Default is 4, but we don't have inline asm that matters because we're doing an AST. Maybe comment?
Same for MinInstAlignment.

(a bunch of others have OK defaults)

Should we do anything about Code16Directive / Code32Directive / Code64Directive? They don't make sense for us, so default is OK.

(a bunch of others have OK defaults)

What about UseIntegratedAssembler?

26

It seems better to have .align be bytes? The historical confusion is annoying, so I'd just force people to use .balign and .p2align anyways, but I think in general folks expect .align to be bytes?

30

Move this up, so it's in the same order as declared in the struct.

36

The defaults are "L", which is the dot needed?

lib/Target/WebAssembly/TargetInfo/WebAssemblyTargetInfo.cpp
29

add "(little endian)" to the description.

lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
43

return_fatal_error in this file too.

lib/Target/WebAssembly/WebAssemblyFrameLowering.h
11

CodingStandards.rst does say that \file and \brief should be in every file header. If we're trying to be on top of the standard then other files should also have this (it looks like you followed the local convention per-file, instead of blindly doing all-doxygen).

31

Could you add a FIXME to implement assignCalleeSavedSpillSlots and getCalleeSavedSpillSlots? It looks like it's something we'll eventually want.

lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
38

OptimizeForSize instead?

lib/Target/WebAssembly/WebAssemblyInstrAtomics.td
41

Redundant.

lib/Target/WebAssembly/WebAssemblyRegisterInfo.h
28

No need for private: here.

lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
49

There doesn't seem to be a default mangling for datalayout, ELF seems like what we want: m:e.
These are the defaults, but would be nice to specify: f32:32-f64:64 ?

sunfish added inline comments.Jun 29 2015, 4:01 PM
CODE_OWNERS.TXT
70

There doesn't seem to be a precedent for having multiple code owners. I think this is something we should talk with others in the LLVM community about first. Fortunately, one doesn't need to be a code owner to review patches, so I don't think we need to block on this.

include/llvm/ADT/Triple.h
89

Other targets that don't have a big-endian variant don't do this.

156–157

This is following the convention of using WebAssembly in capital-letters contexts, and wasm in lower-letters contexts.

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
36

This code is totally unusable without this function, so it doesn't really matter. Crashing is better because we get a stack trace to where we need to write code.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp
25

Ok, I set PointerSize and CalleeSaveStackSlotSize. I added a comment for MaxInstLength. MinInstAlignment seems fine with the default for now. I think we're ok with the defaults for the .code directives (which we probably won't use). I added a comment for UseIntegratedAssembler.

26

I'm all for forcing people to use .p2align/.balign; is that an option? Otherwise, looking at LLVM's other targets, making .align be .p2align seems fairly popular.

30

Ok, sorted.

36

"L" as a symbol prefix is an ugly name mangling because it requires all C symbols be mangled too (since 'L' is a valid C identifier character). I've now changed this to be "", and we can plan to use directives to set symbol visibilities rather than prefixes.

lib/Target/WebAssembly/TargetInfo/WebAssemblyTargetInfo.cpp
29

Other targets that don't have big-endian variants don't do this.

lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
43

As above. Nothing works if this doesn't work.

lib/Target/WebAssembly/WebAssemblyFrameLowering.h
11

I missed adding \file and \brief in InstPrinter/*. Fixed now.

31

It's not clear yet whether we'll use the code that uses these. We can add them later if we need them.

lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
38

ForCodeSize is the name used in a few other places in LLVM. It's not just OptimizeForSize but also MinSize.

lib/Target/WebAssembly/WebAssemblyInstrAtomics.td
41

Fixed.

lib/Target/WebAssembly/WebAssemblyRegisterInfo.h
28

Fixed.

lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
49

The default is MM_None, which is what I think we really want for now, unless we decide for sure we're using ELF.

I like leaving f32/f64 as defaults because it keeps the string shorter, and it makes it easier to see what the more interesting parts are.

sunfish updated this revision to Diff 28725.Jun 29 2015, 4:08 PM

Update per jfb's review comments.

jfb added a comment.Jun 29 2015, 4:16 PM

No strong preference on the last comments, would be nice to fix, but lgtm either way.

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp
36

llvm_unreachable will only crash in debug, though?

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCAsmInfo.cpp
26

I don't think it's an option... Asking folks around me they expect .balign behavior.

sunfish abandoned this revision.Jul 2 2015, 8:40 PM