This is an archive of the discontinued LLVM Phabricator instance.

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.

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

Why static?

112–113

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

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

134

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

574

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

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.

454

Why do you need this change?

468

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

My bad, removed.

112–113

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.

574

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

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!

454

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.

468

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

UB if those bits are not stripped out.

I don't see how this is relevant here.

lib/Analysis/AliasAnalysis.cpp
267

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

360

This change seems unnecessary.

377

This change seems unnecessary.

404

This change seems unnecessary.

lib/Analysis/AliasAnalysisEvaluator.cpp
35

This change seems unnecessary.

258

This change seems unnecessary.

283

This change seems unnecessary.

328

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

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

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

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.