Page MenuHomePhabricator

[MemorySSA] Fix for non-determinism in codegen
ClosedPublic

Authored by mgrang on Nov 15 2016, 3:02 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mgrang updated this revision to Diff 78081.Nov 15 2016, 3:02 PM
mgrang retitled this revision from to [MemorySSA] Fix for non-determinism in codegen.
mgrang updated this object.
mgrang updated this object.Nov 15 2016, 3:09 PM

First, thanks for doing this.

lib/Transforms/Utils/MemorySSA.cpp
1472 ↗(On Diff #78081)

I can find nowhere where this can possibly matter.
The uses in calculate are:
for (BasicBlock *BB : *DefBlocks) {

  if (DomTreeNode *Node = DT.getNode(BB))
    PQ.push(std::make_pair(Node, DomLevels.lookup(Node)));
}

(the PQ is ordered anyway, and in the end, the end result ordering is irrelevant) ...)
...

if (!DefBlocks->count(SuccBB))

...

I do not believe fixing this or iterateddominancefrontiers makes sense.
We do not want to slow down phi placement, either.
Further, MemorySSA has one phi node per block, so i also can't see how it changes the IR.

I suspect this is papering over something else, so i'll take a look.

Note that we could sort them in bb numbering order in the end if we wanted to anyway, so if we really wanted a deterministic order, we'd do that.

(This is what promotememorytoregister does)

Here's an alternative diff that works and does not require slowing down IDFCalculator (when not all things care about the order of the end result):
https://reviews.llvm.org/differential/diff/78091/

mgrang updated this revision to Diff 78114.Nov 15 2016, 5:43 PM
mgrang added a reviewer: dberlin.
mgrang set the repository for this revision to rL LLVM.

@dberlin I incorporated your patch into mine. Had to fix the unit tests to use the correctly numbered temporaries.

mgrang accepted this revision.Nov 21 2016, 11:35 AM
mgrang added a reviewer: mgrang.

@dberlin LGTM'ed this patch via email.

This revision is now accepted and ready to land.Nov 21 2016, 11:35 AM
This revision was automatically updated to reflect the committed changes.