[PATCH] D121157: [AMDGPU] always use underlying object in the pointsToConstantMemory

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added a comment.

In D121157#3365562 <https://reviews.llvm.org/D121157#3365562>, @arsenm wrote:

> In D121157#3365557 <https://reviews.llvm.org/D121157#3365557>, @rampitec wrote:
>
>> In D121157#3365555 <https://reviews.llvm.org/D121157#3365555>, @arsenm wrote:
>>
>>> In D121157#3365553 <https://reviews.llvm.org/D121157#3365553>, @rampitec wrote:
>>>
>>>> In D121157#3365531 <https://reviews.llvm.org/D121157#3365531>, @arsenm wrote:
>>>>
>>>>> In D121157#3365530 <https://reviews.llvm.org/D121157#3365530>, @rampitec wrote:
>>>>>
>>>>>> @arsenm Do you think it makes sense to check if getUnderlyingObject actually brought one of: Argument, GlobalValue, or ConstantAggregate?
>>>>>
>>>>> I would assume the base implementation handles that, but we're already checking for GlobalValue
>>>>
>>>> isConstant is not the same as constant address space though. Then for argument a qualifier const also does not replace constant.
>>>
>>> The point of the method is to return if the memory is modified, not if it's constant address space
>>
>> Right, but people use constant address space to tell it is not modified.
>
> Which is redundant with the constant modifier on a global variable. The address space is superfluous here. Loads can be marked with invariant metadata too

For global variable many people use just constant w/o const. For argument using const does not help at all w/o noalias, and even with noalias outside of a kernel. Then the invariant metadata will result in the same regression as an invariant load can be rescheduled virtually anywhere.

I see 2 options to fix the bug:

1. Go with this patch potentially checking what getUnderlyingObject brought;
2. Change AMDGPUPromoteKernelArguments to attach noclobber metadata instead of a cast. That promise will hold unlike an invariant.


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

https://reviews.llvm.org/D121157