This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add null streamer support
ClosedPublic

Authored by aheejin on Nov 16 2018, 4:26 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Nov 16 2018, 4:26 PM
aheejin updated this revision to Diff 174479.Nov 16 2018, 4:27 PM
  • delete newlines
aheejin updated this revision to Diff 174483.Nov 16 2018, 4:51 PM
  • - Add __stack_pointer generation to test + add comment
aheejin updated this revision to Diff 174484.Nov 16 2018, 4:53 PM
  • Delete an assert for debugging
eush added a comment.Nov 16 2018, 11:39 PM

Thank you! I think this is a better way to support MCNullStreamer than D54614.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
97 ↗(On Diff #174484)

What is virtual here for? It seems to be redundant since these functions are already declared virtual in the base class (hence the override).

test/CodeGen/WebAssembly/null-streamer.ll
12 ↗(On Diff #174484)

Should there be a similar comment about .functype to explain g?

aheejin updated this revision to Diff 174516.Nov 17 2018, 2:31 PM
aheejin marked an inline comment as done.
  • Address comments
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
97 ↗(On Diff #174484)

Thanks, I blindly copy-pasted this from the parent class and forgot to delete them.

test/CodeGen/WebAssembly/null-streamer.ll
12 ↗(On Diff #174484)

Actually we are planning changes (D54652, which will soon be landed) so that not only declared functions but also defined function will generate .functype directive. But anyway I think as you suggested having a second RUN line would be better than comments.

aheejin updated this revision to Diff 174517.Nov 17 2018, 2:32 PM
  • Delete unnecessary comments
eush accepted this revision.Nov 17 2018, 9:10 PM

LGTM

This revision is now accepted and ready to land.Nov 17 2018, 9:10 PM
This revision was automatically updated to reflect the committed changes.