[PATCH] D109337: [GlobalOpt][FIX] Do not embed initializers into AS!=0 globals

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXTargetTransformInfo.h:52
+  bool canHaveNonUndefGlobalInitializerInAddressSpace(unsigned AS) const {
+    return AS != AddressSpace::ADDRESS_SPACE_SHARED;
+  }
----------------
jdoerfert wrote:
> tra wrote:
> > jdoerfert wrote:
> > > tra wrote:
> > > > While we should not have globals with ADDRESS_SPACE_LOCAL  or ADDRESS_SPACE_PARAM, it may be worth to return false for them, too.
> > > That sounds to me more like an assertion. I'll add one for now, you let me know if that is ok too.
> > Assertions are for compiler programming errors, not for diagnosing invalid user-provided input.
> > Considering that one can write IR with a global var in an invalid AS I think assertion is not the right choice here.
> > An error would be more appropriate, IMO.
> TBH, I think such an input is broken and the verifier should trigger, which it does not: https://godbolt.org/z/xr1v361Wo
> Assuming the IR is "valid" we should not misuse this interface with an AS that is not allowed for globals, IMHO.
> That said, I'm not even sure what "an error" would look like here.
Right. TTI just does not have enough context to tell whether this particular call indicates a problem. Perhaps the best we can do here is to just provide the answer whether the AS can/can't handle global initializers and leave it up to the callers to deal with overall IR correctness.

In this particular patch, returning false for local/param AS would be valid. It will block the optimization and that would the the correct action regardless of soundness of the IR we're optimizing. Improving target-specific IR checks in the verifier would be nice, but it's beyond the scope of this patch and should be done separately.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109337