After looking quickly at the patch, I think it is actually fairly reasonable. The complexity seems to be mostly contained inside mips-specific code. In fact, it does not even add an "if MIPS" (it actually removes one). However, I can see how this would add extra complexity to parallelizing, so we probably want to understand the interaction there better.<br><br>It seems this feature is needed for LLD to be a production MIPS linker, so we should support it. We just need to think about how best to do it so that it doesn't become too much a burden. I think we should consider this patch on its own merits and independently from things like the "isMIPS64EL" craziness.<div><br></div><div>-- Sean Silva</div><div><br>On Monday, April 10, 2017, Rui Ueyama via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>On Mon, Apr 10, 2017 at 5:13 PM, Davide Italiano <span dir="ltr"><<a href="javascript:_e(%7B%7D,'cvml','davide@freebsd.org');" target="_blank">davide@freebsd.org</a>></span> wrote:<br></div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On Mon, Apr 10, 2017 at 4:58 PM, Rui Ueyama via llvm-commits<br>
<<a href="javascript:_e(%7B%7D,'cvml','llvm-commits@lists.llvm.org');" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> On Sat, Apr 8, 2017 at 12:08 AM, Simon Atanasyan <<a href="javascript:_e(%7B%7D,'cvml','simon@atanasyan.com');" target="_blank">simon@atanasyan.com</a>><br>
> wrote:<br>
>><br>
>> On Tue, Apr 4, 2017 at 9:09 PM, Rui Ueyama <<a href="javascript:_e(%7B%7D,'cvml','ruiu@google.com');" target="_blank">ruiu@google.com</a>> wrote:<br>
>> > On Tue, Apr 4, 2017 at 5:46 AM, Simon Atanasyan <<a href="javascript:_e(%7B%7D,'cvml','simon@atanasyan.com');" target="_blank">simon@atanasyan.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Sat, Apr 1, 2017 at 4:20 AM, Rui Ueyama via Phabricator<br>
>> >> <<a href="javascript:_e(%7B%7D,'cvml','reviews@reviews.llvm.org');" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br>
>> >> ><br>
>> >> > This is not your fault, but I have to say that this MIPS GOT layout<br>
>> >> > is<br>
>> >> > very odd,<br>
>> >> > too different from other architectures, and too complicated. I want<br>
>> >> > to<br>
>> >> > avoid supporting<br>
>> >> > this unless I'm convinced that it is absolutely necessary. It seems<br>
>> >> > to<br>
>> >> > me that MIPS<br>
>> >> > needs a clean, common new ABI. Only the MIPS ABI imposes a lot of<br>
>> >> > restrictions<br>
>> >> > on the size of GOT sections and the order of GOT section members,<br>
>> >> > even<br>
>> >> > though MIPS<br>
>> >> > as a processor is an ordinary RISC ISA. This change would really hurt<br>
>> >> > maintainability<br>
>> >> > of LLD which I already found some MIPS-specific behavior is hard to<br>
>> >> > keep<br>
>> >> > it right<br>
>> >> > when editing code for all the other architectures.<br>
>> >><br>
>> >> MIPS will not always use old, obsoleted ABIs. It will switch to new<br>
>> >> one. But it does not<br>
>> >> happen this year or so. Besides other obstacles, there is a hardware<br>
>> >> problem prevents from<br>
>> >> fast switching and common acceptance of the new ABI. Historically many<br>
>> >> MIPS instructions<br>
>> >> are partitioned as 16 bit for opcode and 16 bit bit for address/index.<br>
>> >> That is one of<br>
>> >> the source of GOT size limitation and reason of multi-GOT invention.<br>
>> >><br>
>> >> The biggest part of the patch isolated in the MipsGotSection class. It<br>
>> >> adds some new<br>
>> >> MIPS specific code like new constructor of the DynamicReloc class. But<br>
>> >> at the same<br>
>> >> time it removes some `if (Config->EMachine == EM_MIPS)` statements and<br>
>> >> MIPS specific<br>
>> >> fields from the `SymbolBody` class.<br>
>> ><br>
>> ><br>
>> > It is isolated as a separate class, but we still need to understand and<br>
>> > modify it when we need to do something for relocation processing. I'm<br>
>> > actually trying to change the design of relocation processing, to<br>
>> > increase<br>
>> > parallelism of relocation processing. We can't parallelize it entirely,<br>
>> > but<br>
>> > some part (such as making a decision whether a symbol needs a GOT slot<br>
>> > or<br>
>> > not) can be processed per-file or per-relocation basis.<br>
>> ><br>
>> > Then I found that this part of code is very complex and has grown<br>
>> > organically. I tried to reduce its complexity and found that keeping<br>
>> > everything right for MIPS is hard. I'm really don't want to increase<br>
>> > complexity of this code. If you increase the complexity, I won't be able<br>
>> > to<br>
>> > refactor it anymore because I'm struggling to do that even for the<br>
>> > current<br>
>> > code.<br>
>> ><br>
>> > In addition to that, the MIPS multi-GOT ABI doesn't seem a right design<br>
>> > to<br>
>> > me. If multi-GOT is in use, only the first GOT is recognized as a real<br>
>> > GOT<br>
>> > by the dynamic linker, and secondary GOTs are just some sections that<br>
>> > simulates GOT. It's too hacky, isn't it?<br>
>> ><br>
>> >> > I wonder what is the performance penalty you would have to pay when<br>
>> >> > you<br>
>> >> > use the -mxgot<br>
>> >> > option. With the option, you'll need three instructions as opposed to<br>
>> >> > a<br>
>> >> > single instruction<br>
>> >> > to access an GOT entry. Does that actually make observable difference<br>
>> >> > in<br>
>> >> > performance?<br>
>> >><br>
>> >> Regular (without -mxgot) access to GOT requires a single instruction:<br>
>> >><br>
>> >> lw t9,0(gp)<br>
>> >><br>
>> >> I was wrong when say about two instructions. With -mxgot option we get<br>
>> >> three instructions.<br>
>> >><br>
>> >> lui at,0x0<br>
>> >> addu at,at,gp<br>
>> >> lw t9,0(at)<br>
>> >><br>
>> >> In case of MIPS global offset table is used not only to call external<br>
>> >> functions / access<br>
>> >> external data but for local calls / access under some conditions. So<br>
>> >> using -mxgot we can<br>
>> >> easily grow the code size and reduce performance.<br>
>> ><br>
>> ><br>
>> > How much is the actual performance hit?<br>
>><br>
>> Multi-GOT is an attempt to bypass say limitation of MIPS architecture.<br>
>> It's not my invention, this feature was implemented in GNU linker more<br>
>> than ten years ago. Every time when GOT exceeds ~64KB limit BFD and<br>
>> gold linkers create multi-GOT layout.<br>
>><br>
>> I do not think that my implementation of multi-GOT makes LLD much more<br>
>> complicated. General idea remains the same - collect information about<br>
>> various type of required GOT entries, layout GOT entries, write this<br>
>> layout. Merging multiple GOT created for each file into larger GOT is<br>
>> rather complicated routine though. From another side, creating a<br>
>> separate GOT for each input file makes possible to parallelize this<br>
>> process. Current implementation, where MipsGotSection maintains a<br>
>> single `GotEntries` vector for all files, does not allow to process<br>
>> multiple input files at the same time without some sort of "locks".<br>
><br>
><br>
> I understand that you are just trying to implement a MIPS ABI, and I also<br>
> understand that you made your effort to write good code. Your code seems to<br>
> be a straightforward implementation of the ABI if I understand it correctly.<br>
> But still new code inevitably adds complexity, and that's particularly true<br>
> for this patch that introduces a new notion of "multi-GOT" only for MIPS.<br>
> Also, it is not my fault to say that this feature is too odd, because I<br>
> think it's a consequence of MIPS ABI's peculiarities. I believe many<br>
> peculiarities in the MIPS ABI could have been fixed by now since they were<br>
> implemented more than 10 years ago.<br>
><br>
> I really do not want to add this much complexity to our relocation<br>
> processing code which is already too complicated. Even I don't understand<br>
> the exact behavior of the current code, and I'm am trying to refactor that<br>
> code now. This patch could make my refactoring impossible.<br>
><br>
>> Performance degradation in case of using -mxgot depends on<br>
>> application. My tests show that application use -mxgot slower on<br>
>> 1%-4%. But it's more important that there are large applications which<br>
>> cannot be linked without multi-GOT at all even if they built with<br>
>> -mxgot option. Because there are some relocations which operate by<br>
>> 16-bit GOT index only.<br>
>><br>
<br>
</div></div>I don't necessarily disagree, but what you're saying is true for a<br>
bunch of features in MIPS, which is the in-tree lld backend with more,<br>
let's say, "peculiarities". I'm under the impression that as a project<br>
we should make a call and decide whether we want to support the MIPS<br>
ABI entirely or just don't support it at all (unless there's an OK<br>
reason to drop some features). In this case, unless I'm reading the<br>
patch incorrectly, this is needed to link/self-host clang/lld, so this<br>
feature seems needed (in some form). What do you think?<br></blockquote><div><br></div><div>We should support MIPS, but my point is that 1) the current code is already complicated that even I do not understand, and 2) this patch will likely to make it impossible to hack anymore because of an additional complexity. So adding this code right now is not a good idea. I want to explore other way to circumvent this ABI, and if it is impossible to do, I want to implement it in a really good way, probably after cleaning up the current code with the multi-GOT in mind.</div><div><br></div><div>Also, the MIPS ABI needs updating. Only MIPS is very weird among ELF ABIs. Someone really needs to make an effort to streamline it. I'm ok to accept MIPS peculiarities if it is transient ("transient" can be like 10 years), but if no effort is being made to make it compatible with other ELF ABIs, I think I can say "no, this really needs fixing."</div></div></div></div>
</blockquote></div>