This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add more utility functions
ClosedPublic

Authored by aheejin on Jun 16 2018, 8:55 PM.

Details

Summary

Added more utility functions that will be used in EH-related passes. Also
changed LoopBottom function to getBottom and uses templates to be
able to handle other classes as well, which will be used in CFGSort later.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Jun 16 2018, 8:55 PM
aheejin edited the summary of this revision. (Show Details)Jun 17 2018, 2:14 AM
aheejin edited the summary of this revision. (Show Details)Jun 18 2018, 11:02 AM
dschuff added inline comments.Jun 18 2018, 3:16 PM
lib/Target/WebAssembly/WebAssemblyUtilities.cpp
123 ↗(On Diff #151632)

How about just return 0 here and return 1 below, with an unreachable as the default?

lib/Target/WebAssembly/WebAssemblyUtilities.h
44 ↗(On Diff #151632)

typo: 'instruction' and line 47

58 ↗(On Diff #151632)

by 'exception' do you mean try block, catch block, both? what can 'something else' be?

aheejin updated this revision to Diff 151812.Jun 18 2018, 4:09 PM
aheejin marked 2 inline comments as done.
  • Address comments
lib/Target/WebAssembly/WebAssemblyUtilities.h
58 ↗(On Diff #151632)

I meant WebAssemblyException class defined in D44134. Actually this name itself is little confusing, because what it really means is the 'catch' of a wasm exception. I thought about renaming it to WebAssemblyCatch, WebAssemblyExceptionCatch, WebAssemblyEHScope, etc, but none of them really sounded better than this to me. (WebAssemblyEHScope is actually more confusing, because the definition of it is not really same with that of EHScope in general sense.)

Here 'something else' meaning, it can possibly be used for another data structure other than loop or exception if we get to have one. Maybe it sounds confusing and is better to be deleted..?

dschuff accepted this revision.Jun 18 2018, 5:17 PM

LGTM with the tweak to the comment.

lib/Target/WebAssembly/WebAssemblyUtilities.h
58 ↗(On Diff #151632)

Maybe just say "either a loop or the catch block of a WebAssemblyException". I'd leave out the "something else" until we have a concrete use, or maybe a more concrete descriptor (e.g. something that will become encompassed by a wasm control-flow construct?).

This revision is now accepted and ready to land.Jun 18 2018, 5:17 PM
aheejin added inline comments.Jun 18 2018, 5:25 PM
lib/Target/WebAssembly/WebAssemblyUtilities.h
58 ↗(On Diff #151632)

'catch block of a WebAssemblyException' is confusing too because WebAssemblyException itself is only for a catch part. Should we go with just WebAssemblyException?

dschuff added inline comments.Jun 18 2018, 5:26 PM
lib/Target/WebAssembly/WebAssemblyUtilities.h
58 ↗(On Diff #151632)

Yeah that sounds fine. In the other CL we should make it clear what WebAssemblyException actually refers to.

aheejin updated this revision to Diff 151834.Jun 18 2018, 5:33 PM
  • Comment fix
This revision was automatically updated to reflect the committed changes.
aheejin marked 2 inline comments as done.Jun 18 2018, 5:40 PM