[PATCH] D76894: [GlobalOpt/GlobalStatus] Handle GlobalVariables passed as function call operands with access attributes

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D76894#1946810 <https://reviews.llvm.org/D76894#1946810>, @ddcc wrote:

> Sure, I'm working on a instrumentation pass which is inserting calls that inhibit optimization, so I'm trying to work around the issue using function attributes, and need to look into memory to register promotion next.


Interesting. Feel free to send me an email to bounce of ideas :)
You should also enable the Attributor once you start adding attributes ;)

> I've made the changes, but I'm a little confused why read-only and write-only calls need to be handled differently.

Because we use the fact that something was written for reasoning, at least in globalopt. If the global was always written before it was read we basically privatize it in the function. We cannot ensure that it is actually written for write-only calls. Read only doesn't matter because we don't care if it may or must happen. Does that make sense? (Btw. I'm not an expert on this but I just read the surrounding code so I might be wrong.)

> Why can't I assume that in the worst-case, the entire type is accessed?

The worst-case is fine but for a call it is: the entire type is read and something is written.

> Also, what are the semantics of `readonly` and `writeonly` with respect to pointer casts? Isn't it valid behavior in C to cast any pointer type to `void *` as long as it is casted back to the original type before being accessed, so wouldn't this affect both loads and stores (that the actual load/store size could be larger than the argument type size)?

C doesn't really matter here but what you say is not wrong. We cannot conclude anything from the type of the pointer that goes into a call. That is why I said we have to assume the entire array is accessed. However,

  // Assume that in the worst case, the entire type is accessed
  Loads.push_back({CB, U->getType()->getPointerElementType()});

is not it. This uses the type at the call site. The entire thing works because we see the allocation so we know how big the entire thing is. Use the allocation type instead please.
(FWIW, even in C there is no need to cast it back into "the original type" in a lot of situations, including but not limited to access via `char*`.)



================
Comment at: llvm/lib/Transforms/Utils/GlobalStatus.cpp:186
+                   CB->paramHasAttr(ArgNo, Attribute::WriteOnly))
+            GS.StoredType = GlobalStatus::Stored;
+          else
----------------
Are we sure `Stored` means "maybe stored"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76894