Page MenuHomePhabricator

[SelectionDAGBuilder][WebAssembly] Omit range checks on jump tables
AbandonedPublic

Authored by tlively on May 28 2020, 9:43 PM.

Details

Summary

Jump tables on most platforms do not have a default branch target
built in, so LLVM emits a range check before each jump table as long
as the default target for the jump table is reachcable. WebAssembly,
however, implements jump tables using the br_table instruction, which
takes a default branch target as one of its arguments. Since
br_table handles branching to the default target when the argument is
out of range, the range check that LLVM emits is redundant.

This patch adds a TargetLowering setting that disables range checks on
jump tables and adds the default branch target to the end of the
branch target list for each jump table. This allows the WebAssembly
backend to take full advantage of the br_table instruction.

Diff Detail

Event Timeline

tlively created this revision.May 28 2020, 9:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2020, 9:43 PM
hans added a comment.May 29 2020, 2:33 AM

Before digging into the code, would it be impractical to fold the range check into the br_table instruction in the wasm backend instead, "sniffing out the guard" and replacing it as suggested by that comment in WebAssemblyISelLowering.cpp?

Because I'm a little bit worried about this ipushing complexity up to the general switch lowering mechanism, for example breaking the invariant that jump tables have a size that matches their corresponding case range.

tlively planned changes to this revision.May 29 2020, 10:25 AM

Before digging into the code, would it be impractical to fold the range check into the br_table instruction in the wasm backend instead, "sniffing out the guard" and replacing it as suggested by that comment in WebAssemblyISelLowering.cpp?

Because I'm a little bit worried about this ipushing complexity up to the general switch lowering mechanism, for example breaking the invariant that jump tables have a size that matches their corresponding case range.

That's a fair concern. I'll try that approach and report back.

tlively abandoned this revision.May 30 2020, 1:00 AM

Doing this in the WebAssembly backend did not turn out to be as bad as I thought it might be. It is more complex than this solution, but probably more robust and certainly less intrusive as well. I'll close this review in favor of https://reviews.llvm.org/D80863.

hans added a comment.Jun 2 2020, 10:29 AM

Doing this in the WebAssembly backend did not turn out to be as bad as I thought it might be. It is more complex than this solution, but probably more robust and certainly less intrusive as well. I'll close this review in favor of https://reviews.llvm.org/D80863.

Sounds good. Thanks!