<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jul 30, 2015 at 10:55 PM Filipe Cabecinhas <<a href="mailto:filcab%2Bllvm.phabricator@gmail.com">filcab+llvm.phabricator@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">filcab planned changes to this revision.<br>
filcab added a comment.<br>
<br>
Thank you, Eric.<br>
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.<br>
<br></blockquote><div><br></div><div>Awesome.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br></blockquote><div><br></div><div>Seems fair.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Driver/Tools.cpp:3115-3118<br>
@@ -3106,6 +3114,6 @@<br>
<br>
- // Introduce a Darwin-specific hack. If the default is PIC but the flags<br>
- // specified while enabling PIC enabled level 1 PIC, just force it back to<br>
- // level 2 PIC instead. This matches the behavior of Darwin GCC (based on my<br>
- // informal testing).<br>
- if (PIC && getToolChain().getTriple().isOSDarwin())<br>
+ // Introduce a Darwin and PS4-specific hack. If the default is PIC but<br>
+ // the flags specified while enabling PIC enabled level 1 PIC, just<br>
+ // force it back to level 2 PIC instead. This matches the behavior of<br>
+ // Darwin GCC (based on my informal testing).<br>
+ if (PIC && (getToolChain().getTriple().isOSDarwin() ||<br>
----------------<br>
echristo wrote:<br>
> If you could rewrite and separate this out a bit more it'd be great.<br>
I'm not the best at it, but I'll try :-)<br>
<br></blockquote><div><br></div><div>Sure. Happy to help.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/Driver/Tools.cpp:3590-3591<br>
@@ -3580,4 +3589,4 @@<br>
Args.ClaimAllArgs(options::OPT_g_flags_Group);<br>
if (Args.hasFlag(options::OPT_gcolumn_info, options::OPT_gno_column_info,<br>
- /*Default*/ true))<br>
+ /*Default*/ !Triple.isPS4CPU()))<br>
CmdArgs.push_back("-dwarf-column-info");<br>
----------------<br>
echristo wrote:<br>
> Hmm?<br>
We have different defaults from other platforms.<br>
<br></blockquote><div><br></div><div>Yeah, not commented (because it's not /*Default*/. Might be nice to refactor this out slightly? It just a little gross at the moment.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/Driver/Tools.cpp:3613<br>
@@ -3603,3 +3612,3 @@<br>
// backend.<br>
- if (Args.hasArg(options::OPT_gdwarf_aranges)) {<br>
+ if (Args.hasArg(options::OPT_gdwarf_aranges) || Triple.isPS4CPU()) {<br>
CmdArgs.push_back("-backend-option");<br>
----------------<br>
echristo wrote:<br>
> Ditto.<br>
Ditto, different defaults.<br>
But I guess I can hoist out the Triple.isPS4CPU() on both cases, if you prefer, like it's done for other toolchains like:<br>
bool IsWindowsMSVC = getToolChain().getTriple().isWindowsMSVCEnvironment();<br>
<br></blockquote><div><br></div><div>Seems reasonable. I'm not sure how else to refactor it out well.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/Driver/Tools.cpp:9435-9436<br>
@@ +9434,4 @@<br>
+<br>
+ const char *Exec =<br>
+ Args.MakeArgString(getToolChain().GetProgramPath("ps4-as"));<br>
+ C.addCommand(llvm::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));<br>
----------------<br>
echristo wrote:<br>
> Using an external assembler rather than the integrated one?<br>
We support both (but default to integrated, since we don't override `Generic_GCC::IsIntegratedAssemblerDefault()`).<br></blockquote><div><br></div><div>Oh right, and it's just a different name. OK, that makes sense, thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Driver/Tools.h:783<br>
@@ -782,1 +782,3 @@<br>
<br>
+namespace PS4cpu {<br>
+class LLVM_LIBRARY_VISIBILITY Assemble : public Tool {<br>
----------------<br>
echristo wrote:<br>
> Hrm. PS4 maybe? PS4cpu seems to say that it's a custom cpu target and not a custom environment?<br>
PS4cpu (or PS4CPU) is more consistent with how we've been naming things in open source.<br>
<br></blockquote><div><br></div><div>Okie.</div><div><br></div><div>Thanks!</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11279&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=d3JGVyv1FP2PrxvesUC-xCis_V7LMq4ZGxNbxeJ3CNI&s=pc9Krpfi7fxh4R9p2XApUqADvh0U24GK6-VqqBzEJJU&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11279</a><br>
<br>
<br>
<br>
</blockquote></div></div>