Page MenuHomePhabricator

Fix for PR24179 - mem2reg generates incorrect code
ClosedPublic

Authored by gilr on Jul 20 2015, 8:38 AM.

Details

Summary

Patch implements the logic described in main comment of promoteSingleBlockAlloca for loads not preceded by a store:

  • If there are no stores before the load, it is (still) replaced by an undef (test2 in the added lit test)
  • IF there are any stores, but none preceding the load we bail out and let the rest of Mem2Reg's machinery handle this alloca, as the load may be affected by a following store, depending on the CFG (test1 in the added lit test).

Diff Detail

Repository
rL LLVM

Event Timeline

gilr updated this revision to Diff 30161.Jul 20 2015, 8:38 AM
gilr retitled this revision from to Fix for PR24179 - mem2reg generates incorrect code.
gilr updated this object.
gilr added reviewers: sanjoy, mkuper.
gilr added a subscriber: llvm-commits.
sanjoy accepted this revision.Jul 20 2015, 9:24 AM
sanjoy edited edge metadata.

LGTM with comments addressed.

lib/Transforms/Utils/PromoteMemoryToRegister.cpp
429 ↗(On Diff #30161)

I'd make this comment more explicit now:

/// alloca is undefined on only some control flow paths.  e.g. code like
/// this is correct in LLVM IR:
///
///  // A is an alloca with no stores so far
///  for (...) {
///    int t = *A;
///    if (!first_iteration)
///      use(t);
///    *A = 42;
///  }
///
476 ↗(On Diff #30161)

I'd put the StoresByIndex.empty special case here -- that way all of the logic dealing with "cannot find a previous store" lives at one place.

568 ↗(On Diff #30161)

I'd use && here.

This revision is now accepted and ready to land.Jul 20 2015, 9:24 AM
This revision was automatically updated to reflect the committed changes.