Page MenuHomePhabricator

Modify ModRefInfo values using static inline method abstractions [NFC].
ClosedPublic

Authored by asbirlea on Dec 1 2017, 1:06 PM.

Details

Summary

The aim is to make ModRefInfo checks and changes more intuitive
and less error prone using inline methods that abstract the bit operations.

Ideally ModRefInfo would become an enum class, but that change will require
a wider set of changes into FunctionModRefBehavior.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Dec 1 2017, 1:06 PM
asbirlea updated this revision to Diff 125237.Dec 1 2017, 3:38 PM

clang-format.

asbirlea updated this revision to Diff 125255.Dec 1 2017, 9:29 PM

Replace all bit-wise operations involving ModRefInfo with inline method wrappers.

There is nothing (as of this patch) preventing bit-wise operations from being added instead of using the wrappers.

sanjoy requested changes to this revision.Dec 4 2017, 1:55 PM

Hi,

I have some minor comments inline.

I only skimmed through the parts that seemed to be mechanical transformations, but please let me know if there are specific places that should review more carefully.

include/llvm/Analysis/AliasAnalysis.h
111 ↗(On Diff #125255)

Why static?

112–113 ↗(On Diff #125255)

isModOrRef, isMod etc. (alternatively isModOrRefSet or isModSet) sound more idiomatic to me.

[edit: I also have another note later on the setXXX naming].

134 ↗(On Diff #125255)

We should stick to the <verb><noun> format here too, so unionModRef and intersectModRef.

553 ↗(On Diff #125255)

This does not seem NFC to me -- doesn't getModRefBehavior return a FunctionModRefBehavior with the extra bits set? Or is the semantics of enum such that ModRefInfo will strip the unnecessary bits?

lib/Analysis/AliasAnalysis.cpp
151 ↗(On Diff #125255)

One tricky aspect here is that setModAndRef(XX) sounds a bit like "change XX in place and set that bit" when the semantics is more like getWithModAndRefSet, but that's a bit verbose. What do you think? I personally think the verbosity is worth it, but you're touching a lot of this code so I'll leave this to your intuition.

We should also at least mark these "setters" as WARN_UNUSED_VALUE since setModAndRef(XX); (or some other spelling) with no assignment of the result is certainly a bug.

452 ↗(On Diff #125255)

Why do you need this change?

470 ↗(On Diff #125255)

Same here -- this change seems unnecessary.

This revision now requires changes to proceed.Dec 4 2017, 1:55 PM
asbirlea updated this revision to Diff 125435.Dec 4 2017, 3:55 PM
asbirlea edited edge metadata.
asbirlea marked 4 inline comments as done.

Address comments.

Thanks for the review, Sanjoy! Much appreciated!

include/llvm/Analysis/AliasAnalysis.h
111 ↗(On Diff #125255)

My bad, removed.

112–113 ↗(On Diff #125255)

Changing to isModSet makes sense, happy to make that change.
isMod is not the same, however. The goal is to check if it modifies, versus whether it *only* modifies.
Concretely, isModSet will check MRI & MRI_Mod, versus isMod should check MRI==MRI_Mod.

553 ↗(On Diff #125255)

Should have researched this more before hand, yes, I assumed ModRefInfo would strip the unnecessary bits. While this happens to work, it's UB according to the standard.
Correct way is to mask out unnecessary bits before creating a ModRefInfo.
Replacing with a wrapper and adding comment to document this.

lib/Analysis/AliasAnalysis.cpp
151 ↗(On Diff #125255)

I wouldn't really mind the verbosity, but it feels so much simpler to read setMod than getWithModSet. I made arguments const and added LLVM_NODISCARD.
Thanks a lot for the suggestions!

452 ↗(On Diff #125255)

Not really needed in this patch. I reverse-created this patch from the one adding MRI_Must, where I need to check the alias result against multiple values. Undo-ing the change in this patch.

470 ↗(On Diff #125255)

Same as above, change undone.

sanjoy accepted this revision.Dec 4 2017, 5:38 PM

lgtm with some comments inline

include/llvm/Analysis/AliasAnalysis.h
232 ↗(On Diff #125435)

UB if those bits are not stripped out.

I don't see how this is relevant here.

lib/Analysis/AliasAnalysis.cpp
267 ↗(On Diff #125435)

Shouldn't this be isMod? Before it was checking that the return value of getArgModRefInfo was == MRI_Mod.

360 ↗(On Diff #125435)

This change seems unnecessary.

377 ↗(On Diff #125435)

This change seems unnecessary.

404 ↗(On Diff #125435)

This change seems unnecessary.

lib/Analysis/AliasAnalysisEvaluator.cpp
35 ↗(On Diff #125435)

This change seems unnecessary.

258 ↗(On Diff #125435)

This change seems unnecessary.

283 ↗(On Diff #125435)

This change seems unnecessary.

328 ↗(On Diff #125435)

This change seems unnecessary.

This revision is now accepted and ready to land.Dec 4 2017, 5:38 PM
asbirlea updated this revision to Diff 125471.Dec 4 2017, 10:29 PM
asbirlea marked 9 inline comments as done.

Address comments.

Thank you for the comments and the review! I will land this tomorrow.

include/llvm/Analysis/AliasAnalysis.h
232 ↗(On Diff #125435)

The comment was meant so possible future changes don't assume ModRefInfo(FMRB) will drop most significant bits. Removing and hoping that doesn't happen :).

lib/Analysis/AliasAnalysis.cpp
267 ↗(On Diff #125435)

I made this change intentionally, here's why:

Before, it was checking == MRI_Mod and then resetting mask to MRI_ModRef. If result of getArgModRefInfo was already MRI_ModRef, it was left unchanged, but this doesn't stand out.

With this change, it's obvious that if the result of getArgModRefInfo is either MRI_ModRefor MRI_Mod, then the mask should be MRI_ModRef. The comment I added above also highlights this: that if CS2 mods the location, we're looking to further check if CS1 reads or writes, so mask should should be MRI_ModRef.

521 ↗(On Diff #125435)

Undone this too.

asbirlea updated this revision to Diff 125473.Dec 4 2017, 10:44 PM

Missed nits (stray comment, braces).

This revision was automatically updated to reflect the committed changes.