This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix SSA rebuilding in SjLj transformation
ClosedPublic

Authored by aheejin on Aug 26 2019, 1:48 AM.

Details

Summary

Previously we skipped uses within the same BB as a def when rebuilding
SSA after SjLj transformation. For example, before transformation,

for.cond:
  %0 = phi i32 [ %var, %for.inc ] ...
  %var = ...
  br label %for.inc

for.inc:                               ; preds = %for.cond
  call i32 @setjmp(...)
  br %for.cond

In this BB, %var should be defined in all paths from %for.inc to make %0
valid. In the input it was true; %for.inc's only predecessor was
%for.cond. But after SjLj transformation, it is possible that %for.inc
has other predecessors that are reachable without reaching %for.cond.

entry.split:
  ...
  br i1 %a, label %bb.1, label %for.inc

for.cond:
  %0 = phi i32 [ %var, %for.inc ] ...  ; Not valid!
  %var = ...
  br label %for.inc

for.inc:                               ; preds = %for.cond, %entry.split
  call i32 @setjmp(...)
  ...
  br %for.cond

In this case, we can't use %var in the phi instruction in %for.cond,
because %var is not defined in all paths through %for.inc (If the
control flow is %entry -> %entry.split -> %for.inc -> %for.cond, %var
has not been defined until we reach the phi). But the previous code
excluded users within the same BB, skipping instructions within the same
BB so they are not rewritten properly. User instructions within the same
BB also should be candidates for rewriting if they are _before_ the
original definition.

Fixes PR43097.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Aug 26 2019, 1:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 1:48 AM
aheejin edited the summary of this revision. (Show Details)Aug 26 2019, 1:48 AM
aheejin marked an inline comment as done.
aheejin added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
640 ↗(On Diff #217092)

This is just hoisting, because we were doing the same thing unnecessarily repeatedly in the inner loop. Not related to the bugfix itself.

aheejin edited the summary of this revision. (Show Details)Aug 26 2019, 1:50 AM
aheejin edited the summary of this revision. (Show Details)Aug 26 2019, 1:59 AM
aheejin edited the summary of this revision. (Show Details)
dschuff accepted this revision.Aug 26 2019, 10:25 AM
dschuff added inline comments.
llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll
245 ↗(On Diff #217092)

Should this be uncommented or removed?

This revision is now accepted and ready to land.Aug 26 2019, 10:25 AM
aheejin marked an inline comment as done.Aug 26 2019, 2:46 PM
aheejin added inline comments.
llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll
245 ↗(On Diff #217092)

Oh no, after the transformation, for.inc itself will be split into multiple BBs, so the CHECK line below will be in for.inc.split BB.

This revision was automatically updated to reflect the committed changes.