On 07/01/11 19:28:34, Joseph S. Myers wrote:
> On Fri, 1 Jul 2011, Gary Funck wrote:
> GF: * Most of the #ifdef conditionals have been removed. Some target macros
> GF: have been defined and documented in tm.texi. We still have some questions
>
> [...]
> I looked at the first of the documented macros, and it doesn't seem to be
> a target macro at all, being defined by configure rather than in tm.h.
> Configure-defined macros don't go in tm.texi. But any that are really
> target macros need such a justification, and it's still preferable to
> define configure-defined macros to 1 or 0 (rather than defined or
> undefined) and test them in "if" statements not #if.
I was uncertain where/how to document the macros used to configure UPC.
(Your explanation in the [...] above was helpful in answering
the technical questions that I had in that regard, thanks.)
The patch submitted for review did document (as target macros)
switches that are set by the 'configure' script. Whether they should
be #defines/not, I'm not sure, and whether they are target specific
I wasn't sure. Hopefully, the following description will provide
enough background to figure out the best way to handle these
configuration items.
The recent changes were motivated by your original suggestion:
* It appears you have a large number of #ifdef conditionals
with no obvious reason for the macros being conditionally
defined or not defined. The use of conditional compilation
like this is deprecated for new code. If something may
vary from target to target, use target hooks, not macros,
and document them in tm.texi (if they are in fact documented
there in something outside this patch, perhaps you need to
post that other patch). Conditions using "if" are strongly
preferred to those using "#if" whenever possible.
In the version of GUPC that your comments above relate to,
the #define's were primarily in config/upc_conf.h:
http://gcc.gnu.org/viewcvs/branches/gupc/gcc/config/upc-conf.h?revision=173527&view=markup&pathrev=173545
Some of those definitions, which list the names of UPC runtime functions
now in upc/rts-names.h:
http://gcc.gnu.org/viewcvs/branches/gupc/gcc/upc/upc-rts-names.h?revision=173837&view=markup
The definitions that are specific to the representation of the UPC
pointer-to-shared (PTS) type that has been configured by the user were
swept under this corner of the rug in upc/upc-pts.h:
http://gcc.gnu.org/viewcvs/branches/gupc/gcc/upc/upc-pts.h?revision=173837&view=markup
The reason that so many #ifdef's and #defines remain is that
I wasn't sure about the best method of parameterizing those values to
avoid the use of conditional compilation. For example, should
a small struct be defined that has fields for the configuration-specific
values?
The switching between the files that implement the packed representation
of a pointer-to-shared and those that implemented the struct representation
used to be done in the UPC Makefile fragment Make-lang.in (lines 61 and 62):
http://gcc.gnu.org/viewcvs/branches/gupc/gcc/upc/Make-lang.in?revision=169939&view=markup&pathrev=173545
In that version of GUPC only one of two files upc-pts-packed.c or
upc-pts-struct.c was compiled, depending upon the configuration switch
that selected between the two representations. Both files defined the
same exported functions.
In an effort to make sure that both implementations are compiled,
a table of PTS operations is defined in the current version of GUPC
in upc-pts.h:
http://gcc.gnu.org/viewcvs/branches/gupc/gcc/upc/upc-pts.h?revision=173837&view=markup
The table of representation-specific functions is filled in with either
the packed PTS routines or the struct representation routines when
the UPC compiler is initialized.
Where the need for this PTS representation specific logic begins
is controlled by the following UPC configure options:
--with-upc-pts={struct,packed}
choose the representation of a UPC pointer-to-shared
--with-upc-pts-vaddr-order={last,first}
choose position of the address field in UPC
pointer-to-shared representation
--with-packed-upc-pts-bits=phase,thread,vaddr
choose bit distribution in packed UPC
pointer-to-shared representation
The set of configuration choices looks like this:
PTS Representation
Packed
Vaddr: {first, last}
field sizes: {phase,thread,vaddr}
Struct
Vaddr: {first, last}
Although the PTS representation is not target-specific, it has some
aspects of being target-specific in that the representation and
layout of the PTS changes the binary representation of the PTS
in the compiled program (and runtime), and some representations
will perform better/worse on a specific target (for example,
on some targets some UPC programs may run faster if vaddr is first.
Generally, the 'struct PTS' is chosen because it provides the
maximum range of the various fields (at the expense of using
more space to represent the PTS).
Based upon your suggestions/comments, it sounds like:
1. The PTS-related #define's set by 'configure' are not
target macros, and should not be documented as such.
2. The PTS-related operations that are specific either to
the packed or the struct PTS representation are not target
hooks and should not be documented as such.
3. UPC_PTS_VADDR_FIRST currently defined in upc-pts.h:
83 #ifdef HAVE_UPC_PTS_VADDR_FIRST
84 #define UPC_PTS_VADDR_FIRST 1
85 #else
86 #define UPC_PTS_VADDR_FIRST 0
87 #endif /* HAVE_UPC_PTS_VADDR_FIRST */
should be set directly to 1 or 0 by 'configure', avoiding the
need to test HAVE_UPC_PTS_VADDR_FIRST with an #ifdef.
4. The #define's in upc-pts.h that are derived based upon PTS-specific
configuration settings:
#ifdef HAVE_UPC_PTS_PACKED_REP
51 /* 'packed' UPC pointer-to-shared representation */
52 #define UPC_PTS_SIZE 64
53 #else
54 /* 'struct' UPC pointer-to-shared representation */
55 #define UPC_PTS_SIZE (LONG_TYPE_SIZE + POINTER_SIZE)
56 #define UPC_PTS_PHASE_SIZE (LONG_TYPE_SIZE/2)
57 #define UPC_PTS_THREAD_SIZE (LONG_TYPE_SIZE/2)
58 #define UPC_PTS_VADDR_SIZE POINTER_SIZE
59 #define UPC_PTS_PHASE_TYPE ((LONG_TYPE_SIZE == 64) ? "uint32_t" :
"uint16_t")
60 #define UPC_PTS_THREAD_TYPE ((LONG_TYPE_SIZE == 64) ? "uint32_t" :
"uint16_t")
61 #define UPC_PTS_VADDR_TYPE "char *"
62 #endif /* HAVE_UPC_PTS_PACKED_REP */
63
64 #ifdef HAVE_UPC_PTS_VADDR_FIRST
65 #define UPC_PTS_PHASE_SHIFT 0
66 #define UPC_PTS_THREAD_SHIFT UPC_PTS_PHASE_SIZE
67 #define UPC_PTS_VADDR_SHIFT (UPC_PTS_PHASE_SIZE+UPC_PTS_THREAD_SIZE)
68 #else
69 #define UPC_PTS_PHASE_SHIFT (UPC_PTS_VADDR_SIZE+UPC_PTS_THREAD_SIZE)
70 #define UPC_PTS_THREAD_SHIFT UPC_PTS_VADDR_SIZE
71 #define UPC_PTS_VADDR_SHIFT 0
72 #endif
73 #if HOST_BITS_PER_WIDE_INT == HOST_BITS_PER_LONG
74 #define UPC_PTS_VADDR_MASK ((1L << UPC_PTS_VADDR_SIZE) - 1L)
75 #elif HOST_BITS_PER_WIDE_INT == HOST_BITS_PER_LONGLONG
76 #define UPC_PTS_VADDR_MASK ((1LL << UPC_PTS_VADDR_SIZE) - 1LL)
77 #else
78 #error Unexpected "HOST_BITS_PER_WIDE_INT" value.
79 #endif /* HOST_BITS_PER_WIDE_INT */
80 #define UPC_PTS_THREAD_MASK ((1 << UPC_PTS_THREAD_SIZE) - 1)
81 #define UPC_PTS_PHASE_MASK ((1 << UPC_PTS_PHASE_SIZE) - 1)
Should be moved into a 'struct' that is filled in with the UPC
compiler is initialized. This can be done by the representation
specific 'init' routine.
Does that make sense?
- Gary
