[PATCH] D158603: [AMDGPU][TargetMachine] Handle case when +extended-image-insts is set, and the user forces +wave64

[Juan Manuel Martinez Caamaño via Phabricator via llvm-commits]

jmmartinez added a comment.

In D158603#4609902 <https://reviews.llvm.org/D158603#4609902>, @arsenm wrote:

> First, last I knew comgr was not using clang/-mlink-builtin-bitcode (although maybe this was fixed?). 
> [...]
> @uses_nothing isn't even getting the +wavefrontsize64, which is doubly broken. I believe this was working correctly at one point, so did this regress? clang is broken here, so that should be fixed first. If comgr is still not correctly using -mlink-builtin-bitcode, it should implement an approximately equivalent hack to append the features if there are still blockers to doing so.

Thanks for pointing this out. Comgr uses the `Linker::linkInModule` function directly. I wasn't aware that there was a difference, but I see now that using `-mlink-builtin-bitcode` does `Gen->CGM().addDefaultFunctionDefinitionAttributes(F);` and does internalization for every function in the builtin bitcode. `Linker::linkInModule` only keeps the attributes in the definition coming in from the builtin bitcode and ignores those in the declaration.

However, there is a TODO in that function...

  void CodeGenModule::addDefaultFunctionDefinitionAttributes(llvm::Function &F) {
    llvm::AttrBuilder FuncAttrs(F.getContext());
    getDefaultFunctionAttributes(F.getName(), F.hasOptNone(),
                                 /* AttrOnCallSite = */ false, FuncAttrs);
    // TODO: call GetCPUAndFeaturesAttributes?
    F.addFnAttrs(FuncAttrs);
  }

If I set up a call to `GetCPUAndFeaturesAttributes(GlobalDecl(), F)` any target-features present in the definition are overriden (so it overrides the +extended-image-insts).

---

I have one question though. Why device_libs has that target specific attribute? Shouldn't it be deduced later in the optimization pipeline since the function has calls to image intrinsics?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158603