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