[PATCH] D11279: Initial patch for PS4 toolchain

[Eric Christopher]

On Thu, Jul 30, 2015 at 10:55 PM Filipe Cabecinhas <
filcab+llvm.phabricator at 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150803/ff801939/attachment.html>