This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] replaced .param/.result by .functype
ClosedPublic

Authored by aardappel on Nov 16 2018, 2:36 PM.

Details

Summary

This makes it easier/cleaner to generate a single signature from
this directive. Also:

  • Adds the symbol name, such that we don't depend on the location of this directive anymore.
  • Actually constructs the signature in the assembler, and make the assembler own it.
  • Refactor the use of MVT vs ValType in the streamer and assembler to require less conversions overall.
  • Changed 700 or so tests to use it.

Diff Detail

Repository
rL LLVM

Event Timeline

aardappel created this revision.Nov 16 2018, 2:36 PM
sbc100 accepted this revision.Nov 16 2018, 2:56 PM

Nice! I wish we could do with without the lexer change and with a more asm-linux syntax.. but I guess that time for bikeshedding that is over.

lib/MC/MCParser/AsmLexer.cpp
653 ↗(On Diff #174453)

Is there a risk we could break other asm backends with this?

This revision is now accepted and ready to land.Nov 16 2018, 2:56 PM
aardappel marked an inline comment as done.Nov 16 2018, 3:13 PM
aardappel added inline comments.
lib/MC/MCParser/AsmLexer.cpp
653 ↗(On Diff #174453)

If there's other backend that allow parsing a - token followed by > token with no whitespace in between, then we will break them, yes. I think this is highly unlikely, as I don't know of any kind of expression where that could occur.

dschuff accepted this revision.Nov 16 2018, 4:11 PM

otherwise LGTM!

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
189 ↗(On Diff #174453)

at least the operator == overload on StringRef makes this code not as bad as it could be?

377 ↗(On Diff #174453)

why set and then reset the symbol type? all of the intervening returns are error conditions, right?

aardappel marked 3 inline comments as done.Nov 16 2018, 5:17 PM
aardappel added inline comments.
lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
189 ↗(On Diff #174453)

Yup, it's ok :)

377 ↗(On Diff #174453)

That looks like a refactoring slip-up, will remove.

aardappel updated this revision to Diff 174488.Nov 16 2018, 5:20 PM
aardappel marked 2 inline comments as done.

Code review fixes: removed redundant setType()

aardappel updated this revision to Diff 174628.Nov 19 2018, 9:12 AM

Fixes because of just introduced WebAssemblyTargetNullStreamer class.

This revision was automatically updated to reflect the committed changes.