This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] MIPS16 TLS support for GCC


Hi Richard,

Sorry for the delay to respond, and thanks for revising and committing
the changes to trunk. The revised changes look much cleaner :)

About the other concerns:

> (2) is interesting if there is also a way to build those MIPS16 libraries
> out of the box.  I'd like such a mechanism to be added at the same time,
> so that the new support is easy to test.  This is still a 4.7 candidate
> if it can be done in time, although it's probably a little tight.

I assume you mean the libgomp patch? That's mostly a potential bug fix;
as for MIP16 multilib additions, so far I haven't added any to the
default mips*-linux* configs, as I guess this should be vendor-stuff
(though the current additions should clear away any obstacles)

> You've given the built-in function the generic name __builtin_thread_pointer.
> I agree that's a good idea, but I think we should also _treat_ it as a generic
> function, and make it available on all targets.  The actual implementation
> can be delegated to a target hook.  That makes (3) 4.8 material.

Actually, I named and added __builtin_thread_pointer() only because it
was needed for building glibc (to avoid using 32-bit asm in MIPS16
mode), and because some other targets also have it (I'm sure ARM also
has it, and at least one other)

> (Incidentally, I don't think it's correct to define the builtin if TLS
> isn't available, so if we did keep it as a MIPS-specific function,
> d->avail would need to be nonnull.  There would need to be some other
> way of indicating that MIPS16 was OK.)

The function is essentially a wrapper for mips_get_tp(), which I don't
see any guarding for the no TLS case? FWIW, TARGET_HAVE_TLS is just
defined as HAVE_AS_TLS for MIPS...

>> The __mips16_rdhwr() function is not intended for general use, just
>> tightly coupled compiler runtime support; therefore, it is only linked
>> statically from libgcc.a, not exported from shared libgcc.
> 
> Well, a fair bit of libgcc is tightly-coupled compiler runtime support.
> I don't really see any need to handle the new function differently from
> the other __mips16 wrappers.  It's not like we're gaining any benefit in
> the PIC call overhead: we can't turn JALRs into branches like we can for
> nearby non-MIPS16-to-non-MIPS16 calls, so PIC calls will still go via
> the GOT.  And we're not gaining any benefit in terms of ABI baggage either.
> Future libgcc(.a)s would need to continue providing the function as
> specified now, in order for future gccs to be able to link library
> archives built with 4.7.
> 
> By making the function hidden, we lose the important ability to replace
> all instances of it.  E.g. it isn't inconceivable that someone will find
> a more efficient way to implement the function in cases where the RDHWR
> is emulated (perhaps on a kernel other than Linux -- this support isn't
> specific to *-linux-gnu).  Being able to replace all instances of a
> function is useful for other things, such as profiling, debugging,
> or working around processor errata.

We touched this internally as well, but interestingly came to the
opposite conclusion :)  We're not sure the __mips16_rdhwr() is
ultimately the best choice of API, plus the non-standardness of the
calling convention, hence simply decided to hide it from libgcc_s.so, in
case we need to change the MIPS16 TLS interface.

On the issue of hiding stuff in mips16.S, it may be suitable to raise
this issue here: there is a problem when MIPS16 hard-float calls go
through PLTs, because the hard-float stub functions in mips16.S use
v0/v1 as scratch registers, the same as MIPS16 PLT sequences generated
by BFD. (I think I described the scenario mostly correct, CCing Maciej
here who worked on this issue)  I think the solution was to mark a large
portion of the mips16.S functions as all hidden.

>> 5) The libgomp MIPS futex.h header has been adjusted; sys_futex0() has
>> been modified to be static, non-inlined, nomips16 under MIPS16 mode (for
>> sake of using 'syscall'). The inline assembly has also been fixed, as
>> Maciej noticed a possible violation of the MIPS syscall restart
>> convention; the 'li $2, #syscall_number' must be right before the
>> syscall insn.
> 
> This change is OK as part of (2).

Okay thanks, I commit this part soon.

Thanks,
Chung-Lin


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]