Page MenuHomePhabricator

[WebAssembly] Fix bug in FixBrTables and use branch analysis utils
ClosedPublic

Authored by tlively on Jun 15 2020, 10:40 PM.

Details

Summary

This commit fixes a bug in the FixBrTables pass in which an
unconditional branch from the switch header block to the jump table
block was not removed before the blocks were combined. The result was
an invalid CFG in the MachineFunction. This commit also switches from
using bespoke branch analysis and deletion code to using the standard
utilities for the same.

Diff Detail

Event Timeline

tlively created this revision.Jun 15 2020, 10:40 PM

ping @aheejin @dschuff it would be good to get this landed ASAP. The bug is breaking both Google code and external code https://bugs.llvm.org/show_bug.cgi?id=46363.

aheejin accepted this revision.Jun 17 2020, 11:21 AM

Sorry for the delayed reply!

llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp
63

Is it possible that both TBB and FBB are true and FBB == MBB?

This revision is now accepted and ready to land.Jun 17 2020, 11:21 AM
tlively marked an inline comment as done.Jun 17 2020, 12:23 PM
tlively added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp
63

Yes, here's what's possible.

'_' is nullptr, 'J' is the jump table block (aka MBB), 'D' is the default block

TBB | FBB | meaning
_ | _ | No default block, header falls through to jump table
J | _ | No default block, header jumps to jump table
D | _ | There is a default block, header falls through to jump table
D | J | There is a default block, header jumps to either default or jump table

aheejin added inline comments.Jun 17 2020, 12:29 PM
llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp
63

Yeah, I'm talking about the last case:
D | J | There is a default block, header jumps to either default or jump table

In this case, not TBB, but FBB should be the default target. I don't think this code does it.

This revision was automatically updated to reflect the committed changes.
tlively marked an inline comment as done.
tlively marked an inline comment as done.Jun 17 2020, 1:09 PM
tlively added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp
63

Actually, I'll add this table as a comment. It seems useful.

63

No, the condition that gets evaluated is whether the index is out of bounds, so the conditional jump (TBB) is to the default block and the unconditional jump (FBB) is to the jump table block (MBB).

aheejin added inline comments.Jun 17 2020, 1:19 PM
llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp
63

I thought it could be more robust to handle both ways, because we were depending on the particular way the branch is generated in SelectionDAG now. But probably it's an overkill. :)

tlively marked an inline comment as done.Jun 17 2020, 1:37 PM
tlively added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp
63

Ah gotcha. I'll add an assert to the else branch as well to make sure we catch this instead of miscompiling if the codegen ever changes.

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

aheejin added inline comments.Jun 17 2020, 2:06 PM
llvm/test/CodeGen/WebAssembly/switch-unreachable-default.ll
65

Unrelated question: why does this switch get split? Because it’s too big? If so, big br_tables are also bad for wasm too?

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

Neat, I'll start using that pre-push hook. Thanks!

tlively marked an inline comment as done.Jun 17 2020, 2:13 PM
tlively added inline comments.
llvm/test/CodeGen/WebAssembly/switch-unreachable-default.ll
65

Right, it's because the case indices are clustered into different groups that are far away from each other. The target-independent codegen has a lot of logic for figuring out how to best split up and group the switch cases to create only reasonably-dense jump tables. This is good for uncompressed WebAssembly as well, although a sparse br_table would still compress well. I don't know if any engines do clever lowerings of br_tables, but we want to make naively lowering them into native jump tables a reasonable option as well, so it is good to have this logic in the tools.

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

@MaskRay How do I get "Reviewed By:" line? I use arc diff but it only puts Reviewers: line. And we don't know who is gonna review the CL until we actually get it.

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

@MaskRay How do I get "Reviewed By:" line? I use arc diff but it only puts Reviewers: line. And we don't know who is gonna review the CL until we actually get it.

arc amend will amend the git description with Reviewed By: among other tags.