On Thu, Jul 30, 2015 at 10:55 PM Filipe Cabecinhas <
filcab+llvm.phabrica...@gmail.com> wrote:
> filcab planned changes to this revision.
> filcab added a comment.
>
> Thank you, Eric.
> We'll address the comments and post back an updated patch. Probably make
> the linker a separate patch and submit it before we update this one.
>
>
Awesome.
> I would prefer to not split this into "add the toolchain, then fill it out
> more" (if possible), since we already have this in our internal branch.
> Having two bigger merges (especially if one is "non-working") or waiting
> until we have everything to do the merge wouldn't be ideal.
>
>
Seems fair.
>
> ================
> Comment at: lib/Driver/Tools.cpp:3115-3118
> @@ -3106,6 +3114,6 @@
>
> - // Introduce a Darwin-specific hack. If the default is PIC but the flags
> - // specified while enabling PIC enabled level 1 PIC, just force it back
> to
> - // level 2 PIC instead. This matches the behavior of Darwin GCC (based
> on my
> - // informal testing).
> - if (PIC && getToolChain().getTriple().isOSDarwin())
> + // Introduce a Darwin and PS4-specific hack. If the default is PIC but
> + // the flags specified while enabling PIC enabled level 1 PIC, just
> + // force it back to level 2 PIC instead. This matches the behavior of
> + // Darwin GCC (based on my informal testing).
> + if (PIC && (getToolChain().getTriple().isOSDarwin() ||
> ----------------
> echristo wrote:
> > If you could rewrite and separate this out a bit more it'd be great.
> I'm not the best at it, but I'll try :-)
>
>
Sure. Happy to help.
> ================
> Comment at: lib/Driver/Tools.cpp:3590-3591
> @@ -3580,4 +3589,4 @@
> Args.ClaimAllArgs(options::OPT_g_flags_Group);
> if (Args.hasFlag(options::OPT_gcolumn_info,
> options::OPT_gno_column_info,
> - /*Default*/ true))
> + /*Default*/ !Triple.isPS4CPU()))
> CmdArgs.push_back("-dwarf-column-info");
> ----------------
> echristo wrote:
> > Hmm?
> We have different defaults from other platforms.
>
>
Yeah, not commented (because it's not /*Default*/. Might be nice to
refactor this out slightly? It just a little gross at the moment.
> ================
> Comment at: lib/Driver/Tools.cpp:3613
> @@ -3603,3 +3612,3 @@
> // backend.
> - if (Args.hasArg(options::OPT_gdwarf_aranges)) {
> + if (Args.hasArg(options::OPT_gdwarf_aranges) || Triple.isPS4CPU()) {
> CmdArgs.push_back("-backend-option");
> ----------------
> echristo wrote:
> > Ditto.
> Ditto, different defaults.
> But I guess I can hoist out the Triple.isPS4CPU() on both cases, if you
> prefer, like it's done for other toolchains like:
> bool IsWindowsMSVC =
> getToolChain().getTriple().isWindowsMSVCEnvironment();
>
>
Seems reasonable. I'm not sure how else to refactor it out well.
> ================
> Comment at: lib/Driver/Tools.cpp:9435-9436
> @@ +9434,4 @@
> +
> + const char *Exec =
> + Args.MakeArgString(getToolChain().GetProgramPath("ps4-as"));
> + C.addCommand(llvm::make_unique<Command>(JA, *this, Exec, CmdArgs,
> Inputs));
> ----------------
> echristo wrote:
> > Using an external assembler rather than the integrated one?
> We support both (but default to integrated, since we don't override
> `Generic_GCC::IsIntegratedAssemblerDefault()`).
>
Oh right, and it's just a different name. OK, that makes sense, thanks.
>
> ================
> Comment at: lib/Driver/Tools.h:783
> @@ -782,1 +782,3 @@
>
> +namespace PS4cpu {
> +class LLVM_LIBRARY_VISIBILITY Assemble : public Tool {
> ----------------
> echristo wrote:
> > Hrm. PS4 maybe? PS4cpu seems to say that it's a custom cpu target and
> not a custom environment?
> PS4cpu (or PS4CPU) is more consistent with how we've been naming things in
> open source.
>
>
Okie.
Thanks!
-eric
>
> http://reviews.llvm.org/D11279
>
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
