[PATCH] D157451: [AMDGPU] Do not deduce access qualifiers from IR attributes

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/hsa-metadata-deduce-ro-arg-v3.ll:26
 }
 
 !0 = !{i32 1, i32 1}
----------------
yaxunl wrote:
> rampitec wrote:
> > cfang wrote:
> > > cfang wrote:
> > > > rampitec wrote:
> > > > > cfang wrote:
> > > > > > rampitec wrote:
> > > > > > > arsenm wrote:
> > > > > > > > cfang wrote:
> > > > > > > > > arsenm wrote:
> > > > > > > > > > cfang wrote:
> > > > > > > > > > > arsenm wrote:
> > > > > > > > > > > > This looks like lost test coverage, need to test the access of the metadata
> > > > > > > > > > > Under V3, if the access is default, it won't print out "default", (will not print out the access qualifier at all).
> > > > > > > > > > > 
> > > > > > > > > > > But I am curious because "noalias readonly %in", why the access is not "readonly"? Does it mean the original access qualifier got lost? 
> > > > > > > > > > the point of this is introspection of the source program, not the actual properties. if someone defines something as readwrite but is only ever read, the optimizer can conclude it's readonly even though it wasn't declared that way
> > > > > > > > > Under V3 or above, it is designed NOT to print out the access qualifier if it is "default". 
> > > > > > > > > Do you want to CHECK-NOT for :access" for the default?
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > -NEXT checks are safer than -NOT
> > > > > > > Right, this can be deduced and had quite positive performance impact on some kernels.
> > > > > > It seems we are using the same AccQual field for two purposes: 1) optimization, 2) get_kernel_arg_info.AccessQualifier, and 
> > > > > > the second one is expected from  getMetadata("kernel_arg_access_qual")
> > > > > This is the same thing, one is because developer marked it (and language allowed this marking), another because we were able to prove it from compiler. Net impact is still the same, RT may omit copying memory before every dispatch.
> > > > We are seeing a conformance test failure with test_api get_kernel_arg_info.
> > > > The test expects access qualifier to be access_none for any kernels other than image and pipes.
> > > > Do you think the checks from this test are not reasonable?  
> > > I meant if the language allows us to change, it should not check for the original ones. 
> > Sigh. I do not see why the same qualifier cannot apply to other types of memory. Readonly is readonly, and the optimization is useful. I understand that the standard did not say it is allowed, but conformance did pass for a long time this optimization is there.
> There is a kernel arg metadata “.actual_access” which may be used instead:
> 
> “.actual_access”	string	 	
> The actual memory accesses performed by the kernel on the kernel argument. Only present if “.value_kind” is “global_buffer”, “image”, or “pipe”. This may be more restrictive than indicated by “.access” to reflect what the kernel actual does. If not present then the runtime must assume what is implied by “.access” and “.is_const” . Values are:
> 
> “read_only”
> “write_only”
> “read_write”
> 
> https://llvm.org/docs/AMDGPUUsage.html#code-object-v3-metadata
Makes senses as long as agreed with RT. However, I would argue this shall be set in the same patch.


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

https://reviews.llvm.org/D157451