[PATCH] D63444: [ThinLTO] Optimize write-only globals out

[Steven Wu via Phabricator via llvm-commits]

steven_wu accepted this revision.
steven_wu added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:165
 struct ValueInfo {
-  PointerIntPair<const GlobalValueSummaryMapTy::value_type *, 2, int>
+  enum Flags { HaveGV = 1, ReadOnly = 2, WriteOnly = 4 };
+  PointerIntPair<const GlobalValueSummaryMapTy::value_type *, 3, int>
----------------
evgeny777 wrote:
> steven_wu wrote:
> > Would it be cleaner if you create a new enum bitfield for access specifier?
> > ```
> > enum Access { ReadWrite = 0, ReadOnly = 1, WriteOnly =2, Invalid =3 };
> > ```
> > This should make all the method below a bit cleaner. 
> I'm not sure if this would make it cleaner. There is also a HaveGV flag in `PointerIntPair`, so to manipulate access bits as suggested you'll have to do something like this:
> ```
> bool isValidAccessSpecifier() const {
>     return (RefAndFlags.getInt() &  (Invalid << 1)) !=  (Invalid << 1);
> }
> void setWriteOnly() {
>     assert(isValidAccessSpecifier());
>     RefAndFlags.setInt(RefAndFlags.getInt() | (WriteOnly << 1));
>  }
> ```
> so technically almost the same as before
My suggestion was something like (simplified):
```
Access getAccess() const { return RefAndFlags.getInt() >> 1; }
void setAccess(Access status) { RefAndFlags.setInt(status <<1) }
bool isReadOnly() const { getAccess() == ReadOnly; }
```
This is has slightly different semantics than the one you have (it overwrites status and cannot get into invalid state). I am fine with current implementation as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63444/new/

https://reviews.llvm.org/D63444