[PATCH] D67409: [RISCV] enable LTO support, pass some options to linker.

[Kuan Hsu Chen (Zakk) via Phabricator via cfe-commits]

khchen marked an inline comment as done.
khchen added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:498
+    CmdArgs.push_back(
+        Args.MakeArgString(Twine("-plugin-opt=-target-abi=") + ABIName));
 }
----------------
efriedma wrote:
> I don't think this change is right.  In general, target features should be encoded in bitcode files.  This allows compiling different files with different target features, and using runtime detection to only run certain codepaths.  And it makes sure that we end up with a sane result if the user doesn't pass target feature flags on the link line.
> 
> Also, it probably isn't appropriate to make target-independent changes in a commit tagged [RISCV]; most people would assume a change marked like that doesn't have target-independent effects.
> 
> (Sorry about the delayed response to this review; I only just ran into this.)
> I don't think this change is right. In general, target features should be encoded in bitcode files. This allows compiling different files with different target features, and using runtime detection to only run certain codepaths. And it makes sure that we end up with a sane result if the user doesn't pass target feature flags on the link line.
> 

I'm curious about your scenario, LTO will link two bitcodes file into one, so which target-features should be kept in final bitcode since different files have different target features? For example, one target features" is "+armv7-a" and another is "+armv8-a". 

I guess maybe your case is they are same target-features in different files, but this patch will overwrite the encoded target-feature as default.

anyway, I agree with you. I found the target features does not encoded in bitcode files when enabling LTO in RISCV, I will fixed it and revert the target feature part, thanks.


> Also, it probably isn't appropriate to make target-independent changes in a commit tagged [RISCV]; most people would assume a change marked like that doesn't have target-independent effects.
> 
sorry,  I will take care of it in next time.

> (Sorry about the delayed response to this review; I only just ran into this.)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67409