Page MenuHomePhabricator

Print OptimizedAccessType in MemorySSA if it is set
ClosedPublic

Authored by aqjune on Jun 6 2018, 5:52 PM.

Details

Summary

This patch makes MemoryUse/MemoryDef of MemorySSA print OptimizedAccessType if they're set.

Any comments are welcome.

Diff Detail

Repository
rL LLVM

Event Timeline

aqjune created this revision.Jun 6 2018, 5:52 PM

Hi, and thanks for this patch!

I'm unsure if we want to print this out for Defs without adding a bit more information to MemoryDef::print. This is because MemoryUses and MemoryDefs both print out their defining accesses, and getOptimizedAccessType() returns information about the optimized access of a Use or Def.

For Uses, the optimized access *is* the defining access, so I think this is a great addition. For Defs, we don't really print whether or not it's optimized at all right now; it seems a bit awkward to say "the optimized Access type is MayAlias, but we're not going to tell you what the optimized access is".

So, please either back out the change to MemoryDef::print, or have MemoryDef::print print out where the Def is optimized to, if it's been optimized. I'm happy with either of these. :)

lib/Analysis/MemorySSA.cpp
1848 ↗(On Diff #150235)

Hm. Does it break anything to make this function a raw_ostream &operator<<(raw_ostream &, AliasResult) that we stick into lib/Analysis/AliasAnalysis.cpp (with a declaration in include/llvm/Analysis/AliasAnalysis.cpp)?

I ask because this seems useful to just about anyone who wants to print an AliasResult. :)

1851 ↗(On Diff #150235)

I suspect this isn't clang-format'ed. If not, please do so.

1858 ↗(On Diff #150235)

Do we need the default case here? If not, please remove it: I'd rather have compilers warn about missing cases if we ever add something else to AliasResult (unlikely, but...)

1880 ↗(On Diff #150235)

Style nit: Please fold this into the if, like so:

if (Optional<AliasResult> AR = getOptimizedAccessType()) {
  // ...
}
1921 ↗(On Diff #150235)

(Same style nit)

aqjune updated this revision to Diff 150669.Jun 10 2018, 6:59 PM
  • Define << operator for AliasResult & let AliasAnalysisEvaluator.cpp use it as well
  • Update printing format of MemoryDef so it prints optimized def as well
aqjune marked 5 inline comments as done.Jun 10 2018, 7:00 PM

Comments applied :)

This LGTM after one last ultra-small nitpick is addressed. I don't see you in the commit logs, so will you need me to commit this for you?

include/llvm/Analysis/AliasAnalysis.h
95 ↗(On Diff #150669)

Nit: I think we generally try to define these out-of-line unless their body is literally { Foo.print(OS); return OS; }. Please do so.

lib/Analysis/AliasAnalysisEvaluator.cpp
153 ↗(On Diff #150669)

Ooh, nice :)

(Sorry for the double-post; one more bit that I noticed scrolling up)

lib/Analysis/MemorySSA.cpp
1878 ↗(On Diff #150669)

(This should probably go before the isOptimized check, so we're consistent with MemoryUses?)

aqjune updated this revision to Diff 151094.Jun 12 2018, 8:15 PM
  • Move definition of operator<< to AliasAnalysis.cpp
  • Update format of MemoryDef print
aqjune marked 2 inline comments as done.Jun 12 2018, 8:17 PM
aqjune added inline comments.
include/llvm/Analysis/AliasAnalysis.h
95 ↗(On Diff #151094)

I was following the way operator<< is defined in Value.h but I'm not against this suggestion. I updated it

lib/Analysis/MemorySSA.cpp
1879 ↗(On Diff #151094)

Makes sense, updated it

Yes, you can commit this patch.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 14 2018, 1:00 PM
Closed by commit rL334760: [MSSA] Print more optimization information (authored by gbiv, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.

This revision was not accepted when it landed; it landed in state Needs Review.

TIL phab will complain about this. To save any code archaeologists some time, this LGTM.

Committed as noted by Phab. Thanks again!