Skip to content

Rework loop statement compilation / general cleanup #1046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Jan 11, 2020
Merged

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Jan 4, 2020

This is an attempt to tackle #1038 which yet again uncovered problems in how we compile for statements. It's still bad but we are missing the necessary infrastructure like a checker. See comments. Perfectly possible that it'll explode still - that flow stuff simply doesn't fit into my head all at once and I guess we'll need something better eventually.

local.get $0
i32.const 0
local.get $1
i32.const 5
i32.eq
select
br_if $continue|0
br_if $for-continue|0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is interesting in that it exits the block anyway. Wondering if Binaryen could somehow optimize all of this away.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 4, 2020

Regarding the comment

      // TODO: For a proper CFG we'd have to take into account that the loop
      // body affects itself when executing at least twice. One way to do this
      // would be to unroll the first iteration if we detect that local states
      // differ before and after the loop executed for the first time, or we
      // can move this logic to a checker. The latter is preferrable because
      // since it can determine such a condition prior we can compile once with
      // the least common denominator of flags (i.e. null and wrap states).

another potential solution could be to recompile the body iff incompatible local states are detected, using the least common denominator. As long as all of this is encapsulated in its own flow (which we can throw away) that should be possible I guess, if only as a temporary solution. Needs some sort of diagnostics deduplication then.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 4, 2020

From a quick check there are just a few occasions in the code where this is relevant. To give an example, in std/util/string.ts's joinBooleanArray, we have

  var value: bool; // wrapped false
  for (let i = 0; i < lastIndex; ++i) {
    // whatever goes here assumes `value` is properly wrapped
    value = load<bool>(dataStart + i); // potentially not wrapped
  }

In this case we only load 0 or 1, so it doesn't matter, but it illustrates what can go wrong.

@MaxGraey MaxGraey mentioned this pull request Jan 4, 2020
@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 5, 2020

Gave the recompile approach a try in the last commit, but found problems with local null states in the process. Appears that Flow#inheritConditional and friends aren't covering these properly, with the conditional variant being merely an unnecessary (and wrong) special case of #inheritMutual where left is this flow. Need to check.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 6, 2020

This has turned into a largish flow and loop overhaul meanwhile. In particular I'm in the process of refactoring stuff to be less smart and more correct. Will have to see how that turns out.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 7, 2020

Last commit contains the progress I have so far, incl.

  • Refactor flow to set all flags explicitly so it never contains unnecessary flags
  • Remove (most, except those I expect to become useful) unused flow flags
  • Properly handle if without else where then sets nonnull
  • Redo while, do and for compilation
  • Diagrams and comments

Still not perfect but it's just so easy to miss something when trying to take all the possible outcomes into account, especially where branch level tree-shaking kicks in and the goal is to use as few flows as possible. Overall direction remains to be less smart but correct, even if that means that untouched output doesn't look as good as before.

end
br $while-continue|0
end
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange one

i32.const 24
i32.shl
i32.const 24
i32.shr_s
Copy link
Member Author

@dcodeIO dcodeIO Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a for (let k: i8 = 0; k < 100; ++k) loop detecting that incrementing an i8 can yield an unwrapped value, hence recompiling with unified wrap states (wrapped before, not wrapped after -> not wrapped) since unaware of the k < 100 limit. As such expected, but also an optimization opportunity.

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 8, 2020

In my initial testing I also noticed that the code example in #1038 leaked memory, but just now got around to look into that. Turned out that array literals were missing an autorelease, only affecting array literals not immediately assigned to a target, happening on

const newFaces = this.buildFaces( nextVertex, /* missing = */ [ nextFace.edge!, nextFace.edge!.next! ] 
);
// __release(missing)

but not on

const notMissing = [ nextFace.edge!, nextFace.edge!.next! ];
const newFaces = this.buildFaces( nextVertex, notMissing);
// __release(notMissing)

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 9, 2020

There's just so much stuff that could need some modernization :)

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 10, 2020

This is becoming somewhat unweildy, hence suggesting to merge and continue from there. Maybe some sort of general modernization PR.

@dcodeIO dcodeIO changed the title Rework for statement compilation Rework loop statement compilation Jan 10, 2020
@dcodeIO dcodeIO changed the title Rework loop statement compilation Rework loop statement compilation / general cleanup Jan 10, 2020
@dcodeIO dcodeIO merged commit 76f4450 into master Jan 11, 2020
@dcodeIO dcodeIO deleted the rework-for branch March 15, 2020 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants