This is an archive of the discontinued LLVM Phabricator instance.

Change code structure of WebAssemblyLowerEmscriptenException pass
ClosedPublic

Authored by aheejin on Aug 16 2016, 4:15 PM.

Details

Summary

This patch changes the code structure of WebAssemblyLowerEmscriptenException pass to support both exception handling and setjmp/longjmp. It also changes the name of the pass and the source file. Depends on D22958.

  1. Change the file/pass name to WebAssemblyLowerEmscriptenExceptions -> WebAssemblyLowerEmscriptenEHSjLj to make it clear that it supports both EH and SjLj
  2. List function / global variable names at the top so they can be changed easily
  3. Some cosmetic changes

Diff Detail

Event Timeline

aheejin updated this revision to Diff 68279.Aug 16 2016, 4:15 PM
aheejin retitled this revision from to Change code structure of WebAssemblyLowerEmscriptenException pass.
aheejin updated this object.
aheejin added reviewers: dschuff, jpp.
aheejin updated this object.Aug 16 2016, 4:16 PM
aheejin updated this revision to Diff 68306.Aug 16 2016, 10:51 PM

Shorten Builder to IRB

aheejin updated this revision to Diff 68310.Aug 16 2016, 11:34 PM

Member variable initialization

dschuff added inline comments.Aug 16 2016, 11:39 PM
lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
121

IIRC LLVM doesn't use bare 'constexpr' because of MSVC 2013 (instead there's LLVM_CONSTEXPR but that expands to nothing in MSVC2013). In any case I don't think it actually buys anything here (and in any case 'constexpr' implies 'const' for objects), so we can probably just make these 'const' and call it a day.

135

Any particular reason this is now all-caps?

317

maybe add a comment why we might want to do CFGSimplify? (having the TODO is fine though)

aheejin updated this revision to Diff 68316.Aug 17 2016, 12:11 AM
aheejin marked an inline comment as done.

Address comments

aheejin marked an inline comment as not done.Aug 17 2016, 12:12 AM
aheejin added inline comments.
lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
121

Done. (I did that just to put the initializer string inside the class)

135

The variable itself is in all caps, that's all. I don't mind reverting it back to ThrewGV, if you prefer it. Do you?

317

I am not sure whether we need to do CFGSimplify or not, so I left it like that. If I add it eventually, I will comment about the reason.
The reason I considered it was llvm-fastcomp does it between exception handling and setjmp/longjmp handling. It has comments about the reasons why it is doing CFGSimplify:
https://github.com/kripken/emscripten-fastcomp/blob/master/lib/Target/JSBackend/JSBackend.cpp#L3742
But I don't really understand what this comment means.

dschuff edited edge metadata.Aug 17 2016, 8:32 AM

LGTM once the style nits are fixed.

lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
121

Ah, I see. Yeah, it's a shame MSVC2013 doesn't properly support constexpr.

135

I see. The LLVM style guide would call for initial caps in variable names even if they refer to all lower-case strings, so I guess we should also use that convention for all upper-case strings too.

317

I see. maybe just make the comment
"TODO: Run CFGSimplify like the emscripten JSBackend?" or something like that then.

aheejin updated this revision to Diff 68474.Aug 17 2016, 7:20 PM
aheejin marked 2 inline comments as done and an inline comment as not done.
aheejin edited edge metadata.

address comments

aheejin marked an inline comment as done.Aug 17 2016, 7:21 PM
aheejin marked an inline comment as done.Aug 17 2016, 7:22 PM
dschuff accepted this revision.Aug 18 2016, 8:35 AM
dschuff edited edge metadata.
This revision is now accepted and ready to land.Aug 18 2016, 8:35 AM
This revision was automatically updated to reflect the committed changes.