[PATCH] D47566: AMDHSA: Code object v3 updates

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle 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));
----------------
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.


https://reviews.llvm.org/D47566