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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
