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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:498
+    CmdArgs.push_back(
+        Args.MakeArgString(Twine("-plugin-opt=-target-abi=") + ABIName));
 }
----------------
khchen wrote:
> 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.)
> 
> 
Target features are encoded into IR on a per-function basis, so we can keep both.

But yes, I'm more worried about the implicit expectation that the target flags are specified on the link line; this isn't reliably true with common build systems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67409