[PATCH] D95638: AMDGPU: Add target id and code object v4 support

[Konstantin Zhuravlyov via Phabricator via llvm-commits]

kzhuravl added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:129
+  // emitFunctionBodyStart?
+  if (getTargetStreamer() && !getTargetStreamer()->getTargetID())
+    initializeTargetID(M);
----------------
arsenm wrote:
> kzhuravl wrote:
> > arsenm wrote:
> > > Why check getTargetStreamer()? Why would htis ever be null? The uses below don't check it
> > Check is here because the following test is passing "-emit-codegen-only" option to cc1, which executes EmitCodeGenOnlyAction, which does not have MC including target streamer.
> > 
> > ```
> > Clang :: Misc/backend-resource-limit-diagnostics.cl
> > ```
> > 
> > The uses below never check it because backend-resource-limit-diagnostics.cl does not specify the os in the triple.
> > 
> > Do you have better suggestions on how to work around it?
> I don't understand why the asm printer would ever execute without the streamer. I don't see why the triple would matter, or why it matters for this specific test. What happens if you add a triple to that test?
> I don't understand why the asm printer would ever execute without the streamer.

-emit-codegen-only option results in no streamer being created

> I don't see why the triple would matter, or why it matters for this specific test. What happens if you add a triple to that test?

If amdhsa or amdpal is not specified, this function is a NOP. If I add amdhsa or amdpal to triple in the backend-resource-limit-diagnostics.cl test, then it segfaults (without D95638)

I have modified the test to not specify -emit-codegen-only option, which allows us to remove the check here.

https://reviews.llvm.org/D96728


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

https://reviews.llvm.org/D95638