[PATCH] D47566: AMDHSA: Code object v3 updates

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp:403-409
+  Streamer.EmitValue(MCBinaryExpr::createSub(
+      MCSymbolRefExpr::create(
+          KernelCodeSymbol, MCSymbolRefExpr::VK_AMDGPU_REL64, Context),
+      MCSymbolRefExpr::create(
+          KernelDescriptorSymbol, MCSymbolRefExpr::VK_None, Context),
+      Context),
+      sizeof(KernelDescriptor.kernel_code_entry_byte_offset));
----------------
kzhuravl wrote:
> t-tye wrote:
> > kzhuravl wrote:
> > > nhaehnle wrote:
> > > > t-tye wrote:
> > > > > nhaehnle wrote:
> > > > > > Isn't the REL64 here redundant? The way I see it, we should either have an //absolute// reference to `(start of kernel code) - (start of kernel descriptor)`, or a //relative// reference to `(start of kernel code) - offsetof(kernel_descriptor_t, kernel_code_entry_byte_offset)`.
> > > > > > 
> > > > > > I'm not deep enough in MC to say for certain which of these is really preferable.
> > > > > The field requires an offset to the entry point so an absolute relocation cannot be used. The Rel64 relocation record is what describes what is needed. Since the kernel descriptor and entry point are now in different sections, a static relocation record is needed so that it can be fixed up when the relocatable code object is linked to make a shared object.
> > > > > 
> > > > > Previously the kernel descriptor was put in the same section as the code, and the offset was "hard-wired" when it was generated.
> > > > Right, I agree that a REL64 relocation is ultimately needed. My point is more about how to express that fact inside LLVM using the MCExpr framework.
> > > > 
> > > > I read the expressing that is being created here as literally `(start of kernel code) - (start of kernel descriptor)`. The fact that it's a relative relocation really ought to be implied by that already, it's not clear to me why VK_AMDGPU_REL64 is passed to one of the constructors in addition to that.
> > > If we just put (start of kernel code) - (start of kernel descriptor), it ends up being R_AMDGPU_ABS64.
> > I agree with @nhaehnle that this seems strange it is needed. Is that a limitation or bug in MCExpr handling? Perhaps it is being used in a way not seen before and so it mishandles this case, and ought to be fixed?
> I put a fixme comment for now.
I think the definition of VK_AMDGPU_REL64 got dropped in the most recent patch?


https://reviews.llvm.org/D47566