Page MenuHomePhabricator

Fix typo in comment for function "static void initializeWorkList" in "/lib/Analysis/CFLAndersAliasAnalysis.cpp"
ClosedPublic

Authored by Enna1 on Feb 27 2019, 6:51 PM.

Details

Summary

It seems that there is a typo in "/lib/Analysis/CFLAndersAliasAnalysis.cpp".
There is a comment in function "static void initializeWorkList":

"If there's an assignment edge from X to Y, it means Y is reachable from X at S2 and X is reachable from Y at S1"

The corrected comment should be:
"If there's an assignment edge from X to Y, it means Y is reachable from X at S3 and X is reachable from Y at S1"

Diff Detail

Repository
rL LLVM

Event Timeline

Enna1 created this revision.Feb 27 2019, 6:51 PM
Enna1 added a comment.Feb 27 2019, 7:16 PM

The definition of function "initializeWorkList" :

static void initializeWorkList(std::vector<WorkListItem> &WorkList,
                               ReachabilitySet &ReachSet,
                               const CFLGraph &Graph) {
  for (const auto &Mapping : Graph.value_mappings()) {
    auto Val = Mapping.first;
    auto &ValueInfo = Mapping.second;
    assert(ValueInfo.getNumLevels() > 0);

    // Insert all immediate assignment neighbors to the worklist
    for (unsigned I = 0, E = ValueInfo.getNumLevels(); I < E; ++I) {
      auto Src = InstantiatedValue{Val, I};
      // If there's an assignment edge from X to Y, it means Y is reachable from
      // X at S3 and X is reachable from Y at S1
      for (auto &Edge : ValueInfo.getNodeInfoAtLevel(I).Edges) {
        propagate(Edge.Other, Src, MatchState::FlowFromReadOnly, ReachSet,
                  WorkList);
        propagate(Src, Edge.Other, MatchState::FlowToWriteOnly, ReachSet,
                  WorkList);
      }
    }
  }
}

MatchState::FlowFromReadOnly represents S1 in the paper "Demand-driven alias analysis for C", MatchState::FlowToWriteOnly and MatchState::FlowToReadWrite represent S3 in the paper.

JonasToth added a comment.EditedFeb 28 2019, 6:45 AM

i have no clue if that is correct. i added some reviews from the CSA guys.

*EDIT*: Please upload the diff with full context.

JonasToth removed a subscriber: JonasToth.
Enna1 updated this revision to Diff 188820.Feb 28 2019, 5:30 PM

That's not Clang though, so i don't know much about it.

I think @george.burgess.iv is the right person because he appears a lot in git blame :)

I think @george.burgess.iv is the right person because he appears a lot in git blame :)

Yeah, I just need to go back through the paper and remember all of this so I can approve this with confidence. :)

Thanks for the patch! I hope to get to it later this week.

george.burgess.iv accepted this revision.Mar 7 2019, 5:52 PM

LGTM; thanks again!

Do you need me to commit this?

This revision is now accepted and ready to land.Mar 7 2019, 5:52 PM
Enna1 added a comment.Mar 8 2019, 12:49 AM

Yes, it's my first time to request a code review.
Thank a lot for your help to commit this.

This revision was automatically updated to reflect the committed changes.