[PATCH, i386]: Introduce QI_REGNO_P macro
Hello!
... to make a couple of trivial cleanups.
2012-08-04 Uros Bizjak
* config/i386/i386.h (QI_REGNO_P): New define.
(ANY_QI_REGNO_P): Ditto.
(GENERAL_REGNO_P): Use IN_RANGE macro.
(QI_REG_P): Use QI_REGNO_P.
(ANY_QI_REG_P): Use GENERAL_REGNO_P and QI_REGNO_P.
(HARD_REGNO_CALLER_SAVE_MODE): Use QI_REGNO_P.
* config/i386/i386.c (ix86_hard_regno_mode_ok): Ditto.
(x86_extended_QIreg_mentioned_p): Ditto. Also check if
register is a general register.
Tested on x86_64-pc-linux-gnu {,-m32} and committed to mainline SVN.
Uros.
Index: config/i386/i386.c
===
--- config/i386/i386.c (revision 190140)
+++ config/i386/i386.c (working copy)
@@ -5402,7 +5402,7 @@ ix86_function_regparm (const_tree type, const_tree
so less registers should be used for argument passing.
This functionality can be overriden by an explicit
regparm value. */
- for (regno = 0; regno <= DI_REG; regno++)
+ for (regno = AX_REG; regno <= DI_REG; regno++)
if (fixed_regs[regno])
globals++;
@@ -32067,7 +32067,7 @@ ix86_hard_regno_mode_ok (int regno, enum machine_m
{
/* Take care for QImode values - they can be in non-QI regs,
but then they do cause partial register stalls. */
- if (regno <= BX_REG || TARGET_64BIT)
+ if (TARGET_64BIT || QI_REGNO_P (regno))
return true;
if (!TARGET_PARTIAL_REG_STALL)
return true;
@@ -33668,8 +33668,8 @@ x86_extended_QIreg_mentioned_p (rtx insn)
int i;
extract_insn_cached (insn);
for (i = 0; i < recog_data.n_operands; i++)
-if (REG_P (recog_data.operand[i])
- && REGNO (recog_data.operand[i]) > BX_REG)
+if (GENERAL_REG_P (recog_data.operand[i])
+ && !QI_REGNO_P (REGNO (recog_data.operand[i])))
return true;
return false;
}
Index: config/i386/i386.h
===
--- config/i386/i386.h (revision 190140)
+++ config/i386/i386.h (working copy)
@@ -1091,7 +1091,7 @@ enum target_cpu_default
: (MODE) == VOIDmode && (NREGS) != 1 ? VOIDmode \
: (MODE) == VOIDmode ? choose_hard_reg_mode ((REGNO), (NREGS), false) \
: (MODE) == HImode && !TARGET_PARTIAL_REG_STALL ? SImode\
- : (MODE) == QImode && (REGNO) > BX_REG && !TARGET_64BIT ? SImode\
+ : (MODE) == QImode && !(TARGET_64BIT || QI_REGNO_P (REGNO)) ? SImode
\
: (MODE))
/* The only ABI that saves SSE registers across calls is Win64 (thus no
@@ -1316,29 +1316,32 @@ enum reg_class
registers. */
#define TARGET_SMALL_REGISTER_CLASSES_FOR_MODE_P hook_bool_mode_true
-#define QI_REG_P(X) (REG_P (X) && REGNO (X) <= BX_REG)
+#define QI_REG_P(X) (REG_P (X) && QI_REGNO_P (REGNO (X)))
+#define QI_REGNO_P(N) IN_RANGE ((N), AX_REG, BX_REG)
-#define GENERAL_REGNO_P(N) \
- ((N) <= STACK_POINTER_REGNUM || REX_INT_REGNO_P (N))
-
#define GENERAL_REG_P(X) \
(REG_P (X) && GENERAL_REGNO_P (REGNO (X)))
+#define GENERAL_REGNO_P(N) \
+ (IN_RANGE ((N), AX_REG, SP_REG) || REX_INT_REGNO_P (N))
-#define ANY_QI_REG_P(X) (TARGET_64BIT ? GENERAL_REG_P(X) : QI_REG_P (X))
+#define ANY_QI_REG_P(X) (REG_P (X) && ANY_QI_REGNO_P (REGNO (X)))
+#define ANY_QI_REGNO_P(N) \
+ (TARGET_64BIT ? GENERAL_REGNO_P (N) : QI_REGNO_P (N))
+#define REX_INT_REG_P(X) (REG_P (X) && REX_INT_REGNO_P (REGNO (X)))
#define REX_INT_REGNO_P(N) \
IN_RANGE ((N), FIRST_REX_INT_REG, LAST_REX_INT_REG)
-#define REX_INT_REG_P(X) (REG_P (X) && REX_INT_REGNO_P (REGNO (X)))
#define FP_REG_P(X) (REG_P (X) && FP_REGNO_P (REGNO (X)))
#define FP_REGNO_P(N) IN_RANGE ((N), FIRST_STACK_REG, LAST_STACK_REG)
+
#define ANY_FP_REG_P(X) (REG_P (X) && ANY_FP_REGNO_P (REGNO (X)))
#define ANY_FP_REGNO_P(N) (FP_REGNO_P (N) || SSE_REGNO_P (N))
#define X87_FLOAT_MODE_P(MODE) \
(TARGET_80387 && ((MODE) == SFmode || (MODE) == DFmode || (MODE) == XFmode))
-#define SSE_REG_P(N) (REG_P (N) && SSE_REGNO_P (REGNO (N)))
+#define SSE_REG_P(X) (REG_P (X) && SSE_REGNO_P (REGNO (X)))
#define SSE_REGNO_P(N) \
(IN_RANGE ((N), FIRST_SSE_REG, LAST_SSE_REG) \
|| REX_SSE_REGNO_P (N))
@@ -1356,13 +1359,13 @@ enum reg_class
(TARGET_FMA4 && ((MODE) == V4SFmode || (MODE) == V2DFmode \
|| (MODE) == V8SFmode || (MODE) == V4DFmode))
-#define MMX_REG_P(XOP) (REG_P (XOP) && MMX_REGNO_P (REGNO (XOP)))
+#define MMX_REG_P(X) (REG_P (X) && MMX_REGNO_P (REGNO (X)))
#define MMX_REGNO_P(N) IN_RANGE ((N), FIRST_MMX_REG, LAST_MMX_REG)
-#define STACK_REG_P(XOP) (REG_P (XOP) && STACK_REGNO_P (REGNO (XOP)))
+#define STACK_REG_P(X) (REG_P (X) && STACK_REGNO_P (REGNO (X)))
#define STACK_REGNO_P(N) IN_RANGE ((N), FIRST_STACK_REG, LAST_STACK_REG)
-#define STACK_TOP_P(XOP) (REG_P (XOP) && REGNO (XOP) == FIRST_STACK_REG)
+#define STACK_TOP_P(X) (
Re: [Patch, fortran] PR fortran/54166 ICE on array section (4.8 regression)
Dear Mikael, This looks to be "obvious" and is certainly OK for trunk. Thanks for the patch. Paul On 3 August 2012 16:18, Mikael Morin wrote: > Hello, > > here is the fix for the regression I have introduced with my assumed > rank bounds patch. > > Will test and commit as obvious. > Mikael > > > 2012-08-02 Mikael Morin > > PR fortran/54166 > * trans-array.c (set_loop_bounds): Access specinfo using spec_dim. > > 2012-08-02 Mikael Morin > > PR fortran/54166 > * gfortran.dg/array_5.f90: New test. > > -- The knack of flying is learning how to throw yourself at the ground and miss. --Hitchhikers Guide to the Galaxy
Re: [RFA, patch] Add missing source location info to thunks
On Sat, Aug 4, 2012 at 1:38 AM, Cary Coutant wrote:
> diff --git a/gcc/final.c b/gcc/final.c
> index 095d608..d22130f 100644
> --- a/gcc/final.c
> +++ b/gcc/final.c
> @@ -1863,11 +1863,12 @@ final (rtx first, FILE *file, int optimize_p)
>start_to_bb = XCNEWVEC (basic_block, bb_map_size);
>end_to_bb = XCNEWVEC (basic_block, bb_map_size);
>
> - FOR_EACH_BB_REVERSE (bb)
> - {
> - start_to_bb[INSN_UID (BB_HEAD (bb))] = bb;
> - end_to_bb[INSN_UID (BB_END (bb))] = bb;
> - }
> + if (cfun->cfg != NULL)
> + FOR_EACH_BB_REVERSE (bb)
> + {
> + start_to_bb[INSN_UID (BB_HEAD (bb))] = bb;
> + end_to_bb[INSN_UID (BB_END (bb))] = bb;
> + }
> }
>
>/* Output the insns. */
Maybe check cfun->is_thunk instead? Everything else should have a cfg.
Ciao!
Steven
Re: Value type of map need not be default copyable
On 08/03/2012 05:19 PM, Ollie Wild wrote: On Fri, Aug 3, 2012 at 2:39 AM, Paolo Carlini wrote: Ok, but, can you also double check and in case fix unordered_map too? Looks like we have the same issue, right? Indeed, we do. I'll send a separate patch for the unordered_map problem. But apparently something doesn't work together with the complex tangle of issues having to do with Core/1402 (aka c++/53733): the new testcase doesn't compile in mainline. Thus, I'm reverting the patch for now. The we'll have to reanalyze whether the library specs should be further tweaked (beyond the container' bits of libstdc++/53657) in the light of Core/1402. Paolo.
Re: Value type of map need not be default copyable
.. note anyway, that only the new testcase was failing, no regressions on pre existing testcases. Thus, it may well be that only the testcase had issues, whereas the code changes themselves (likewise those for unordered_map) are fine as they are, no changes needed elsewhere, neithe in the specs nor in our code. I didn't really further investigate so far (in any case we don't want unexpected fails in the testsuite) Iw would be great if you could get into the details. Thanks, Paolo.
[RFC PATCH, i386]: Improve LIMIT_RELOAD_CLASSES [was: Reload/i386 patch for secondary memory vs subregs]
On Fri, Aug 3, 2012 at 10:37 PM, Uros Bizjak wrote:
>> Without this, on the new testcase we hit the assert in
>> inline_secondary_memory_needed. The comment before the function states:
>>
>> The macro can't work reliably when one of the CLASSES is class
>> containing registers from multiple units (SSE, MMX, integer). We
>> avoid this by never combining those units in single alternative in
>> the machine description. Ensure that this constraint holds to avoid
>> unexpected surprises.
>>
>> So, this indicates that we shouldn't be using INT_SSE_REGS for a reload
>> class at all, and I expect that at the moment we don't. With the patch,
>> the new find_valid_class_1 discovers INT_SSE_REGS as the best class for
>> the register to hold the SYMBOL_REF, leading to the failed assert.
Actually. existing LIMIT_RELOAD_CLASS is way too simple to handle all
issues with mixed register sets. Looking at ix86_hard_regno_mode_ok,
we have problems with DI and SI mode, which can go int XMM and GENERAL
regs, and SF and DF mode, which can go into XMM, FLOAT and GENERAL
regs, depending on the availability of units.
Attached (RFC) patch handles this limitation by limiting multiple
register set modes to the "natural mode" register set, i.e. DI and SI
modes will always return GENERAL_REGS, DF and SF will return either
SSE_REGS, or FLOAT_REGS or GENERAL_REGS. Please note, that we don't
want to widen i.e. CREG or ADREG narrow classes to full GENERAL_REGS.
The patch also improves Q_REGS selection in the same way, and adds a
couple of missing registers to various register sets, so the macro
works as expected.
2012-08-04 Uros Bizjak
* config/i386/i386.h (LIMIT_RELOAD_CLASS): Return preferred
single unit register class for classes that contain registers form
multiple units.
(REG_CLASS_CONTENTS): Add missing "frame" register to FLOAT_INT_REGS,
INT_SSE_REGS and FLOAT_INT_SSE_REGS register classes.
Patch was bootstrapped on x86_64-pc-linux-gnu {,-m32}. Regression test
is in process.
Uros.
Index: i386.h
===
--- i386.h (revision 190141)
+++ i386.h (working copy)
@@ -1297,9 +1297,9 @@
{ 0x1fe00100,0x1fe000 }, /* FP_TOP_SSE_REG */\
{ 0x1fe00200,0x1fe000 }, /* FP_SECOND_SSE_REG */ \
{ 0x1fe0ff00,0x1fe000 }, /* FLOAT_SSE_REGS */\
- { 0x1, 0x1fe0 }, /* FLOAT_INT_REGS */\
-{ 0x1fe100ff,0x1fffe0 }, /* INT_SSE_REGS */ \
-{ 0x1fe1,0x1fffe0 }, /* FLOAT_INT_SSE_REGS */\
+ { 0x11, 0x1fe0 }, /* FLOAT_INT_REGS */\
+{ 0x1ff100ff,0x1fffe0 }, /* INT_SSE_REGS */ \
+{ 0x1ff1,0x1fffe0 }, /* FLOAT_INT_SSE_REGS */\
{ 0x,0x1f }
\
}
@@ -1377,14 +1377,28 @@
/* Place additional restrictions on the register class to use when it
is necessary to be able to hold a value of mode MODE in a reload
- register for which class CLASS would ordinarily be used. */
+ register for which class CLASS would ordinarily be used.
-#define LIMIT_RELOAD_CLASS(MODE, CLASS)\
- ((MODE) == QImode && !TARGET_64BIT \
- && ((CLASS) == ALL_REGS || (CLASS) == GENERAL_REGS \
- || (CLASS) == LEGACY_REGS || (CLASS) == INDEX_REGS) \
- ? Q_REGS : (CLASS))
+ We avoid classes containing registers from multiple units due to
+ the limitation in ix86_secondary_memory_needed. We limit these
+ classes to their "natural mode" single unit register class, depending
+ on the unit availability.
+ Please note that reg_class_subset_p is not commutative, so these
+ conditions mean "... if (CLASS) includes ALL registers from the
+ register set." */
+
+#define LIMIT_RELOAD_CLASS(MODE, CLASS)
\
+ (((MODE) == QImode && !TARGET_64BIT \
+&& reg_class_subset_p (Q_REGS, (CLASS))) ? Q_REGS \
+ : (((MODE) == SImode || (MODE) == DImode) \
+ && reg_class_subset_p (GENERAL_REGS, (CLASS))) ? GENERAL_REGS\
+ : (SSE_FLOAT_MODE_P (MODE) && TARGET_SSE_MATH \
+ && reg_class_subset_p (SSE_REGS, (CLASS))) ? SSE_REGS\
+ : (X87_FLOAT_MODE_P (MODE) \
+ && reg_class_subset_p (FLOAT_REGS, (CLASS))) ? FLOAT_REGS
\
+ : (CLASS))
+
/* If we are copying between general and FP registers, we need a memory
location. The same is true for SSE and MMX registers. */
#define SECONDARY_MEMORY_NEEDED(CLASS1, CLASS2, MODE) \
[C++ Patch] PR 54161
Hi,
as discussed on the audit trail, I'm changing c_sizeof_or_alignof_type
to unconditionally pedwarn in C++ mode. I have to also tweak an existing
testcase, which was exactly relying on the warning to be suppressed by
-Wno-pointer-arith.
Booted and tested x86_64-linux.
Thanks,
Paolo.
/
/c-family
2012-08-04 Paolo Carlini
PR c++/54161
* c-common.c (c_sizeof_or_alignof_type): In C++, pedwarn for function
type and void type without -pedantic and -Wpointer-arith too.
/testsuite
2012-08-04 Paolo Carlini
PR c++/54161
* g++.dg/warn/pr54161.C: New.
* g++.old-deja/g++.jason/ambig2.C: Adjust.
Index: c-family/c-common.c
===
--- c-family/c-common.c (revision 190141)
+++ c-family/c-common.c (working copy)
@@ -4578,10 +4578,17 @@ c_sizeof_or_alignof_type (location_t loc,
{
if (is_sizeof)
{
- if (complain && (pedantic || warn_pointer_arith))
- pedwarn (loc, pedantic ? OPT_Wpedantic : OPT_Wpointer_arith,
-"invalid application of % to a function type");
- else if (!complain)
+ if (complain)
+ {
+ if (c_dialect_cxx ())
+ pedwarn (loc, 0, "invalid application of % to "
+"a function type");
+ else if (pedantic || warn_pointer_arith)
+ pedwarn (loc, pedantic ? OPT_Wpedantic : OPT_Wpointer_arith,
+"invalid application of % to "
+"a function type");
+ }
+ else
return error_mark_node;
value = size_one_node;
}
@@ -4601,12 +4608,17 @@ c_sizeof_or_alignof_type (location_t loc,
}
else if (type_code == VOID_TYPE || type_code == ERROR_MARK)
{
- if (type_code == VOID_TYPE
- && complain && (pedantic || warn_pointer_arith))
- pedwarn (loc, pedantic ? OPT_Wpedantic : OPT_Wpointer_arith,
-"invalid application of %qs to a void type", op_name);
+ if (complain && type_code == VOID_TYPE)
+ {
+ if (c_dialect_cxx ())
+ pedwarn (loc, 0,
+"invalid application of %qs to a void type", op_name);
+ else if (pedantic || warn_pointer_arith)
+ pedwarn (loc, pedantic ? OPT_Wpedantic : OPT_Wpointer_arith,
+"invalid application of %qs to a void type", op_name);
+ }
else if (!complain)
-return error_mark_node;
+ return error_mark_node;
value = size_one_node;
}
else if (!COMPLETE_TYPE_P (type)
Index: testsuite/g++.old-deja/g++.jason/ambig2.C
===
--- testsuite/g++.old-deja/g++.jason/ambig2.C (revision 190141)
+++ testsuite/g++.old-deja/g++.jason/ambig2.C (working copy)
@@ -2,10 +2,8 @@
// { dg-options "-Wno-pointer-arith" }
// Testcase for ambiguity between cast and parmlist.
// This ambiguity accounts for 1 of the r/r conflicts.
-// Do not compile with -pedantic so that the compiler will accept taking
-// the sizeof a function type.
void f(){
(void)sizeof(int((int)1.2));
- (void)sizeof(int((int)));// { dg-bogus "" }
+ (void)sizeof(int((int)));// { dg-warning "invalid application" }
}
Index: testsuite/g++.dg/warn/pr54161.C
===
--- testsuite/g++.dg/warn/pr54161.C (revision 0)
+++ testsuite/g++.dg/warn/pr54161.C (revision 0)
@@ -0,0 +1,12 @@
+// PR c++/54161
+// { dg-options "-Wno-pointer-arith" }
+
+void f();
+void (&g())();
+
+const int a = sizeof(void);// { dg-warning "invalid application" }
+const int b = sizeof(void()); // { dg-warning "invalid application" }
+const int c = sizeof(f()); // { dg-warning "invalid application" }
+const int d = sizeof(g()); // { dg-warning "invalid application" }
+
+typedef char test[a + b + c + d > 0 ? 1 : -1];
Re: Value type of map need not be default copyable
On Sat, 4 Aug 2012, Paolo Carlini wrote: .. note anyway, that only the new testcase was failing, no regressions on pre existing testcases. What I am seeing is a different testcase (with the same name but in a different directory) failing, because: typedef std::pair V; static_assert(std::is_constructible::value,"too bad"); and it makes sense, since you end up having to construct a rvalstruct from a rvalstruct const&&. make_pair constructs a pair without const, from which a pair with const is constructible, though I am surprised it doesn't fail somewhere further. I don't know what the right solution is, maybe something emplace-like. In any case, make_pair is unlikely to be right. -- Marc Glisse
Re: [PATCH, MIPS] 74k madd scheduler tweaks
Sandra Loosemore writes: > The existing scheduler bypass information for madd on the 74k uses some > bits copied from the 24k, and is not quite correct. This patch is based > on one originally sent to us by MIPS and has been present in our local > source base for years. I've confirmed that we are legally allowed to > contribute this to the FSF; ok for mainline? Sorry to ask, but do you have a record of why? Reason I ask is that... > Index: gcc/config/mips/74k.md > === > --- gcc/config/mips/74k.md(revision 189988) > +++ gcc/config/mips/74k.md(working copy) > @@ -168,10 +168,11 @@ > ;; mult/madd/msub->int_mfhilo : 4 cycles (default) > ;; mult->madd/msub : 1 cycles > ;; madd/msub->madd/msub: 1 cycles > -(define_bypass 1 "r74k_int_mult,r74k_int_mul3" "r74k_int_madd" > - "mips_linked_madd_p") > -(define_bypass 1 "r74k_int_madd" "r74k_int_madd" > - "mips_linked_madd_p") > +(define_bypass 1 "r74k_int_mult" "r74k_int_madd") > +(define_bypass 1 "r74k_int_madd" "r74k_int_madd") > + > +(define_bypass 1 "r74k_int_mul3" "r74k_int_madd" > + "mips_mult_madd_chain_bypass_p") > > ;; -- > ;; Floating Point Instructions ...this looks like a step backwards. Before reload, the original version assumes that a multiplication feeding a madd is going to form a chain _as long as_ the result of the multiplication is used as the accumulator input to the madd. The condition is important because something like: r1 = r2 * r3 r6 = r1 * r4 + r5 (which is perfectly OK before reload) shouldn't form a chain. mult and mul3 are equivalent at this stage because we're still using pseudo registers and haven't yet introduced uses of LO. Having both reservations in the same bypass makes sense because of that. The original version should be OK after reload too. It should then never be the case that r74k_int_mul3 feeds r74k_int_madd in a way that satisfies mips_linked_madd_p, so the bypass is harmless but becomes temporarily redundant. But it should always be the case that r74k_int_mult only feeds r74k_int_madd in a way that satisfies mips_linked_madd_p, so the _predicate_ should be harmless but temporarily equivalent to "always true". I.e. the original version should behave as: (define_bypass 1 "r74k_int_mult" "r74k_int_madd") (define_bypass 1 "r74k_int_madd" "r74k_int_madd") after reload (with no bypass for r74k_int_mul3). At least that's how it's supposed to work. The new version is obviously equivalent in the post-reload case, but seems to be making the pre-reload case worse. I'm wondering whether someone hit a situation where mips_linked_madd_p wasn't working properly. Richard
Re: [PATCH, MIPS] clean up 24k/74k store bypasses
Sandra Loosemore writes: > This patch changes the 24k/74k scheduling descriptions to use the existing > mips_store_data_bypass_p predicate instead of treating cprestore as a special > case. OK for mainline? Nice cleanup, thanks. > 2012-08-02 Sandra Loosemore > Maxim Kuvyrkov > Julian Brown > > gcc/ > * config/mips/24k.md (r24k_unknown_store): Delete special handling > for cprestore. > (r24k_int_load, r24k_int_arith, r24k_int_mul3, r24k_int_mfhilo) > (r24k_int_cop, r24k_int_multi) > (r24kf2_1_fcvt_f2i, r24kf2_1_fxfer) > (r24kf1_1_fcvt_f2i, r24kf1_1_fxfer): Use mips_store_data_bypass_p > instead of store_data_bypass_p. > * config/mips/74k.md (r74k_int_store): Delete special handling for > cprestore. > (r74k_int_load, r74k_int_logical, r74k_int_arith, r74k_int_cmove): > Use mips_store_data_bypass_p instead of store_data_bypass_p. OK. Richard
Re: [PATCH, MIPS] DSP ALU scheduling
Sandra Loosemore writes:
> This is another patch that has been present in our local source base for some
> years now. It originally came from MIPS; I've verified that we have legal
> permission to contribute it to the FSF.
>
> The 74k.md parts of this patch depend on the not-yet-reviewed "74k madd
> scheduler
> tweaks" patch I posted the other day:
>
> http://gcc.gnu.org/ml/gcc-patches/2012-08/msg00062.html
>
> Assuming that one gets approved, is this patch OK for mainline?
OK with:
> +/* DSP ALU can bypass data with no delays for the following pairs. */
> +enum insn_code dspalu_bypass_table[][2] =
> +{
> + {CODE_FOR_mips_addsc, CODE_FOR_mips_addwc},
> + {CODE_FOR_mips_cmpu_eq_qb, CODE_FOR_mips_pick_qb},
> + {CODE_FOR_mips_cmpu_lt_qb, CODE_FOR_mips_pick_qb},
> + {CODE_FOR_mips_cmpu_le_qb, CODE_FOR_mips_pick_qb},
> + {CODE_FOR_mips_cmp_eq_ph, CODE_FOR_mips_pick_ph},
> + {CODE_FOR_mips_cmp_lt_ph, CODE_FOR_mips_pick_ph},
> + {CODE_FOR_mips_cmp_le_ph, CODE_FOR_mips_pick_ph},
> + {CODE_FOR_mips_wrdsp, CODE_FOR_mips_insv}
> +};
> +
> +int
> +mips_dspalu_bypass_p (rtx out_insn, rtx in_insn)
> +{
> + int i;
> + int num_bypass = (sizeof (dspalu_bypass_table)
> + / (2 * sizeof (enum insn_code)));
this changed to ARRAY_SIZE (dspalu_bypass_table);
Richard
Re: [ARM] Use UBFX for some and-immediate operations
On 03/08/12 19:10, Richard Henderson wrote: > On 2012-08-02 09:24, Richard Earnshaw wrote: >> +/* Extz only supports SImode, but we can coerce the operands >> + into that mode. */ >> +emit_constant_insn (cond, >> +gen_extzv_t2 (gen_lowpart (mode, target), >> + gen_lowpart (mode, source), >> + GEN_INT (i), const0_rtx)); > > Didn't you mean gen_lowpart (SImode, ...) ? > > > r~ > Urm, yes. Well spotted. Fixed thusly: 2012-08-04 Richard Earnshaw * arm.c (arm_gen_constant): Use SImode when preparing operands for gen_extzv_t2. --- arm.c (revision 190143) +++ arm.c (local) @@ -2999,8 +2999,8 @@ arm_gen_constant (enum rtx_code code, en /* Extz only supports SImode, but we can coerce the operands into that mode. */ emit_constant_insn (cond, - gen_extzv_t2 (gen_lowpart (mode, target), - gen_lowpart (mode, source), + gen_extzv_t2 (gen_lowpart (SImode, target), + gen_lowpart (SImode, source), GEN_INT (i), const0_rtx)); }
Re: [PATCH, MIPS] diagnose -fpic/-fpie incompatibility with -mno-abicalls
Sandra Loosemore writes:
> For all supported MIPS ABIs other than EABI, compiling with
> -fpic/-fPIC/-fpie/-fPIE implicitly requires abicalls support. On Linux
> targets, -mabicalls is the default, but bare-metal targets like
> mips-sde-elf default to -mno-abicalls and naively compiling with -fPIC
> alone doesn't result in working code. (In fact, even "-fPIC -mabicalls"
> doesn't work, because -mabicalls then conflicts with the default for -G,
> and the linker rejects mixing -mabicalls code with -mno-abicalls
> libraries.)
Yeah. I was tempted to add a message for this a while back, but I think
there was resistance from someone who was managing to generate a form of
PIC with careful use of that combination. If they did, it was by accident
rather than design.
However, EABI doesn't support PIC at all. (There used to be an
-membedded-pic option, but that's long gone.) So I think this:
> Index: gcc/config/mips/mips.c
> ===
> --- gcc/config/mips/mips.c(revision 190052)
> +++ gcc/config/mips/mips.c(working copy)
> @@ -15872,6 +15872,8 @@ static void
> mips_option_override (void)
> {
>int i, start, regno, mode;
> + static const char * const mips_abi_name[] =
> +{"-mabi=32", "-mabi=n32", "-mabi=64", "-mabi=eabi", "-mabi=o64"};
>
>if (global_options_set.x_mips_isa_option)
> mips_isa_option_info = &mips_cpu_info_table[mips_isa_option];
> @@ -16053,6 +16055,12 @@ mips_option_override (void)
>target_flags &= ~MASK_ABICALLS;
> }
>
> + /* On non-EABI targets, -fpic and -fpie require -mabicalls. */
> + if (mips_abi != ABI_EABI && (flag_pic || flag_pie) && !TARGET_ABICALLS)
> +error ("the combination of %qs and %qs is incompatible with %qs",
> +(flag_pic ? "-fpic" : "-fpie"),
> +"-mno-abicalls", mips_abi_name[mips_abi]);
should be:
if (flag_pic)
{
if (mips_abi == ABI_EABI)
error ("cannot generate position-independent code for %qs",
"-mabi=eabi");
else if (!TARGET_ABICALLS)
error ("position-independent code requires %qs", "-mabicalls");
}
(where flag_pie implies flag_pic). I've removed the -fpic/-fpie thing
to avoid having to decide between printing "-fpic" and "-fPIC"
(or "-fpie" and "-fPIE").
OK with that change, if you agree.
Richard
Re: [PATCH, MIPS] fix clear cache test cases
Sandra Loosemore writes: > 2012-08-03 Sandra Loosemore > Catherine Moore > > gcc/testsuite/ > * gcc.target/mips/clear-cache-1.c: Test for alternate cache > flush function names too. > * gcc.target/mips/clear-cache-1.c: Likewise. OK, thanks. Richard
Re: [PATCH, MIPS] Netlogic XLR tuning tweaks
Sandra Loosemore writes: > 2012-08-03 Catherine Moore > Sandra Loosemore > > gcc/ > * config/mips/xlr.md (ir_xlr_alu_clz): New insn_reservation. > (ir_xlr_alu): Remove clz. > * config/mips/mips-cpus.def (xlr): Set PTF_AVOID_BRANCHLIKELY. OK, thanks. Richard
Re: Value type of map need not be default copyable
On 08/04/2012 03:26 PM, Marc Glisse wrote: On Sat, 4 Aug 2012, Paolo Carlini wrote: .. note anyway, that only the new testcase was failing, no regressions on pre existing testcases. What I am seeing is a different testcase (with the same name but in a different directory) failing, because: typedef std::pair V; static_assert(std::is_constructible::value,"too bad"); and it makes sense, since you end up having to construct a rvalstruct from a rvalstruct const&&. Oops, you are right, sorry. To be clear, the testcase which was failing with the patch applied is (just check testresults, many examples) is: 23_containers/map/element_access/2.cc make_pair constructs a pair without const, from which a pair with const is constructible, though I am surprised it doesn't fail somewhere further. I don't know what the right solution is, maybe something emplace-like. In any case, make_pair is unlikely to be right. I'm not sure to understand which specific testcase you are discussing, but for sure we don't want regressions. I agree that we should assume a priori that the standard is right, but correcting the make_pair should not lead to failures elsewhere (unless a proper analysis establishes that the existing testcases are wrong) Paolo.
Re: Value type of map need not be default copyable
On Sat, 4 Aug 2012, Paolo Carlini wrote: I'm not sure to understand which specific testcase you are discussing, but for sure we don't want regressions. I agree that we should assume a priori that the standard is right, but correcting the make_pair should not lead to failures elsewhere (unless a proper analysis establishes that the existing testcases are wrong) Let's say it currently works by accident. What I believe is needed is to implement the missing emplace function, and then operator[] and others can be made to use it. -- Marc Glisse
Re: Value type of map need not be default copyable
On 08/04/2012 05:16 PM, Marc Glisse wrote: On Sat, 4 Aug 2012, Paolo Carlini wrote: I'm not sure to understand which specific testcase you are discussing, but for sure we don't want regressions. I agree that we should assume a priori that the standard is right, but correcting the make_pair should not lead to failures elsewhere (unless a proper analysis establishes that the existing testcases are wrong) Let's say it currently works by accident. What I believe is needed is to implement the missing emplace function, and then operator[] and others can be made to use it. Are you really sure that emplace is involved? I'm not. The letter of the standard uses 'inserts'. Paolo.
Re: Value type of map need not be default copyable
On Sat, 4 Aug 2012, Paolo Carlini wrote: On 08/04/2012 05:16 PM, Marc Glisse wrote: On Sat, 4 Aug 2012, Paolo Carlini wrote: I'm not sure to understand which specific testcase you are discussing, but for sure we don't want regressions. I agree that we should assume a priori that the standard is right, but correcting the make_pair should not lead to failures elsewhere (unless a proper analysis establishes that the existing testcases are wrong) Let's say it currently works by accident. What I believe is needed is to implement the missing emplace function, and then operator[] and others can be made to use it. Are you really sure that emplace is involved? I'm not. The letter of the standard uses 'inserts'. The font indicates it is "english" insert, not "function" insert. In the testcase, value_type is not move constructible, so once you have an object of type value_type, you can't do anything with it. -- Marc Glisse
Re: Value type of map need not be default copyable
On 08/04/2012 05:27 PM, Marc Glisse wrote: On Sat, 4 Aug 2012, Paolo Carlini wrote: On 08/04/2012 05:16 PM, Marc Glisse wrote: On Sat, 4 Aug 2012, Paolo Carlini wrote: I'm not sure to understand which specific testcase you are discussing, but for sure we don't want regressions. I agree that we should assume a priori that the standard is right, but correcting the make_pair should not lead to failures elsewhere (unless a proper analysis establishes that the existing testcases are wrong) Let's say it currently works by accident. What I believe is needed is to implement the missing emplace function, and then operator[] and others can be made to use it. Are you really sure that emplace is involved? I'm not. The letter of the standard uses 'inserts'. The font indicates it is "english" insert, not "function" insert. In the testcase, value_type is not move constructible, so once you have an object of type value_type, you can't do anything with it. First, I think we should add libstdc++ in CC. Thus, I would recommend people working in this area to begin with unordered_map, because in that case emplace is already available, assuming that's really the point (and therefore reverting the patch was a good idea, luckily an existing testcase helped us) At the same time an implementation of emplace is definitely welcome, in any case. Paolo.
Re: [libiberty] add obstack macros (was Re: PR #53525 - track-macro-expansion performance regression)
On Fri, 3 Aug 2012, Ian Lance Taylor wrote: 2012-08-04 Dimitrios Apostolou * libiberty.h (XOBDELETE,XOBGROW,XOBGROWVEC,XOBSHRINK,XOBSHRINKVEC): New type-safe macros for obstack allocation. (XOBFINISH): Renamed argument to PT since it is a pointer to T. +/* Type-safe obstack allocator. You must first initialize the obstack with + obstack_init() or _obstack_begin(). This should recommend obstack_init, or obstack_begin, but not _obstack_begin. Also obstack_specify_allocation and obstack_specify_allocation_with_arg are OK, so really it might be better not to list the functions, but simply say "You must first initialization the obstack." Grep reveals that 9 out of 10 times we use _obstack_begin(), and we set alignment to 0 (isn't it suboptimal?). + T: Type, O: Obstack, N: Number of elements, S: raw Size, s/Size/size/ +#define XOBSHRINK(O, T)obstack_blank ((O), -1 * sizeof (T)) +#define XOBSHRINKVEC(O, T, N) obstack_blank ((O), -1 * sizeof (T) * (N)) These are hard to use safely. I'm not sure we should define them at all. I've already used XOBSHRINK and it looks clear to me, but I could use obstack_blank() directly if necessary. +#define XOBFINISH(O, PT) ((PT) obstack_finish ((O))) For XOBNEW, etc., we use (T *) rather than (PT). Using (PT) seems error-probe--it's the only use of the obstack with a different type parameter. Why not use T rather than PT here, and return (T *)? I'd have to change many (about 60) occurences of XOBFINISH if I change that. I'd go for it if I was sure it's what we want, it can be a separate patch later on. Thanks, Dimitris
Re: [patch, fortran] Fix PR 54033, problems with -I, with test cases
On Fri, Aug 3, 2012 at 10:09 AM, Thomas Koenig wrote: > Hi, > >> OK for trunk? > > > I will be on holiday starting tomorrow, so I would like to commit > this today, if possible (with a mistake in the ChangeLog noted > by Uros fixed. Thanks!) > > If not, somebody please commit this (or another fix). > I will commit it after verifying that it fixes the problem. Thanks. -- H.J.
Re: [Patch, fortran] PR fortran/54166 ICE on array section (4.8 regression)
On Fri, Aug 3, 2012 at 7:18 AM, Mikael Morin wrote: > Hello, > > here is the fix for the regression I have introduced with my assumed > rank bounds patch. > > Will test and commit as obvious. > Mikael > > > 2012-08-02 Mikael Morin > > PR fortran/54166 > * trans-array.c (set_loop_bounds): Access specinfo using spec_dim. > > 2012-08-02 Mikael Morin > > PR fortran/54166 > * gfortran.dg/array_5.f90: New test. > > Will this also fix http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54175 -- H.J.
Re: [PATCH, gcc/doc]: Document AMD btver2 enablement
On Sat, 4 Aug 2012, venkataramanan.ku...@amd.com wrote: > Index: gcc/doc/extend.texi > === > +@item btver1 > +AMD family 14h cpu. CPU... > @item amdfam15h > AMD family 15h CPU. ...like you already had here. :-) > +@item btver2 > +AMD family 16h cpu. CPU. > +@item btver2 > +CPUs based on AMD Family 16h cores with x86-64 instruction set support. (This > +supersets MOVBE, F16C, BMI, AVX, PCL_MUL, AES, SSE4.2, SSE4.1, CX16, ABM, > +SSE4A, SSSE3, SSE3, SSE2, SSE, MMX and 64-bit instruction set extensions.) I could not find "supsersets" as a verb in my dictionary: how about "includes"? And I'd omit the parentheses around the second sentence, but this is just a preference on my side; feel free to keep them if you prefer. Okay with these changes. Thanks, Gerald
Re: [PATCH, MIPS] 74k madd scheduler tweaks
On 08/04/2012 07:48 AM, Richard Sandiford wrote: Sandra Loosemore writes: The existing scheduler bypass information for madd on the 74k uses some bits copied from the 24k, and is not quite correct. This patch is based on one originally sent to us by MIPS and has been present in our local source base for years. I've confirmed that we are legally allowed to contribute this to the FSF; ok for mainline? Sorry to ask, but do you have a record of why? Reason I ask is that... Index: gcc/config/mips/74k.md === --- gcc/config/mips/74k.md (revision 189988) +++ gcc/config/mips/74k.md (working copy) @@ -168,10 +168,11 @@ ;; mult/madd/msub->int_mfhilo : 4 cycles (default) ;; mult->madd/msub : 1 cycles ;; madd/msub->madd/msub: 1 cycles -(define_bypass 1 "r74k_int_mult,r74k_int_mul3" "r74k_int_madd" - "mips_linked_madd_p") -(define_bypass 1 "r74k_int_madd" "r74k_int_madd" - "mips_linked_madd_p") +(define_bypass 1 "r74k_int_mult" "r74k_int_madd") +(define_bypass 1 "r74k_int_madd" "r74k_int_madd") + +(define_bypass 1 "r74k_int_mul3" "r74k_int_madd" + "mips_mult_madd_chain_bypass_p") ;; -- ;; Floating Point Instructions ...this looks like a step backwards. [long explanation trimmed] Sigh, I wasn't able to find any detailed rationale for this patch. However, the "DSP ALU scheduling" patch I posted separately gives a clue what the intent was as it also uses mips_mult_madd_chain_bypass_p to give different behavior pre- and post-reload: +;; Before reload, all multiplier is registered as imul3 (which has a long +;; latency). We temporary jig the latency such that the macc groups +;; are scheduled closely together during the first scheduler pass. +(define_bypass 1 "r74k_int_mul3" "r74k_dsp_mac" + "mips_mult_madd_chain_bypass_p") +(define_bypass 1 "r74k_int_mul3" "r74k_dsp_mac_sat" + "mips_mult_madd_chain_bypass_p") If this is incorrect or looks like a hack to paper over some other problem, I'd be happy to drop the predicate on these bits too. WDYT? -Sandra
Re: [PATCH, MIPS] Add 34Kn cpu
On Wed, 1 Aug 2012, Richard Sandiford wrote: >> 2012-08-01 Catherine Moore >> Sandra Loosemore >> >> gcc/ >> * config/mips/mips-cpus.def (34kn): New. >> * config/mips/mips.h (MIPS_ARCH_FLOAT_SPEC): Add 34kn. >> (BASE_DRIVER_SELF_SPECS): Do not imply -mdsp for the 34kn. > Needs adding to invoke.texi too. OK with that change, thanks. And http://gcc.gnu.org/gcc-4.8/changes.html :-) Gerald
[wwwdocs] User generic bug references in GCC 4.5 release notes
On Mon, 23 Apr 2012, Gerald Pfeifer wrote: > This fixes two more occurrences per Rainer's earlier observation. And this should be the last occurrence in our tree. Applied. Gerald Index: htdocs/gcc-4.5/changes.html === RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.5/changes.html,v retrieving revision 1.105 diff -u -3 -p -r1.105 changes.html --- htdocs/gcc-4.5/changes.html 2 Jul 2012 09:11:34 - 1.105 +++ htdocs/gcc-4.5/changes.html 4 Aug 2012 21:13:16 - @@ -103,7 +103,7 @@ GCC has been integrated with the http://www.multiprecision.org/";>MPC library. This allows GCC to evaluate complex arithmetic at compile time http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30789";>more +href="http://gcc.gnu.org/PR30789";>more accurately. It also allows GCC to evaluate calls to complex built-in math functions having constant arguments and replace them at compile time with their mathematically equivalent results. In
[wwwdocs] projects/cxx0x.html Bugzilla reference
Applied. (Of course I realized I missed one after declaring victory just a few minutes ago. ;-) Gerald Index: htdocs/projects/cxx0x.html === RCS file: /cvs/gcc/wwwdocs/htdocs/projects/cxx0x.html,v retrieving revision 1.56 diff -u -3 -p -r1.56 cxx0x.html --- htdocs/projects/cxx0x.html 30 May 2012 13:19:25 - 1.56 +++ htdocs/projects/cxx0x.html 4 Aug 2012 21:15:40 - @@ -187,7 +187,7 @@ Generalized attributes http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2761.pdf";>N2761 - http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53528";>Work in progress + http://gcc.gnu.org/PR53528";>Work in progress Generalized constant expressions
Re: [PATCH, MIPS] Add 34Kn cpu
On 08/04/2012 03:01 PM, Gerald Pfeifer wrote: On Wed, 1 Aug 2012, Richard Sandiford wrote: 2012-08-01 Catherine Moore Sandra Loosemore gcc/ * config/mips/mips-cpus.def (34kn): New. * config/mips/mips.h (MIPS_ARCH_FLOAT_SPEC): Add 34kn. (BASE_DRIVER_SELF_SPECS): Do not imply -mdsp for the 34kn. Needs adding to invoke.texi too. OK with that change, thanks. And http://gcc.gnu.org/gcc-4.8/changes.html :-) Eh, this is pretty trivial compared to some of the other MIPS improvements that are either recently committed or in the pipeline -- specifically, the microMIPS support Catherine is working on, and the recently-committed Netlogic XLP patch. We also have a batch of MIPS16 improvements that have not been submitted yet, including the missing TLS/PIC bits to make MIPS16 work with Linux. (I can't claim to be responsible for any of this good stuff myself, though; I'm just helping with whacking at some smaller patches in our backlog while I'm between other projects.) -Sandra
[C++ Patch] PR 54165
Hi,
Marc noticed that this issue, where we are using an available templated
conversion function for a cast to void, apparently can be rather easily
attacked by moving the convert_to_void case a bit earlier in
build_static_cast_1, that is before the
perform_direct_initialization_if_possible call. I had a cursory look to
other uses of convert_to_void elsewhere, actually moved the code, booted
and tested successfully on x86_64-linux. Makes sense?
Thanks,
Paolo.
///
/cp
2012-08-05 Marc Glisse
Paolo Carlini
PR c++/54165
* typeck.c (build_static_cast_1): Move the conversion to void case
before the perform_direct_initialization_if_possible call.
/testsuite
2012-08-05 Marc Glisse
Paolo Carlini
PR c++/54165
* g++.dg/conversion/void2.C: New.
Index: testsuite/g++.dg/conversion/void2.C
===
--- testsuite/g++.dg/conversion/void2.C (revision 0)
+++ testsuite/g++.dg/conversion/void2.C (revision 0)
@@ -0,0 +1,16 @@
+// PR c++/54165
+
+struct A
+{
+ template
+ operator T()
+ {
+T l[];
+ }
+};
+
+int main()
+{
+ A a;
+ (void)a;
+}
Index: cp/typeck.c
===
--- cp/typeck.c (revision 190144)
+++ cp/typeck.c (working copy)
@@ -6053,6 +6053,12 @@ build_static_cast_1 (tree type, tree expr, bool c_
/* [expr.static.cast]
+ Any expression can be explicitly converted to type cv void. */
+ if (TREE_CODE (type) == VOID_TYPE)
+return convert_to_void (expr, ICV_CAST, complain);
+
+ /* [expr.static.cast]
+
An expression e can be explicitly converted to a type T using a
static_cast of the form static_cast(e) if the declaration T
t(e);" is well-formed, for some invented temporary variable
@@ -6074,12 +6080,6 @@ build_static_cast_1 (tree type, tree expr, bool c_
/* [expr.static.cast]
- Any expression can be explicitly converted to type cv void. */
- if (TREE_CODE (type) == VOID_TYPE)
-return convert_to_void (expr, ICV_CAST, complain);
-
- /* [expr.static.cast]
-
The inverse of any standard conversion sequence (clause _conv_),
other than the lvalue-to-rvalue (_conv.lval_), array-to-pointer
(_conv.array_), function-to-pointer (_conv.func_), and boolean
Re: [PATCH, MIPS] diagnose -fpic/-fpie incompatibility with -mno-abicalls
On 08/04/2012 08:09 AM, Richard Sandiford wrote:
Sandra Loosemore writes:
For all supported MIPS ABIs other than EABI, compiling with
-fpic/-fPIC/-fpie/-fPIE implicitly requires abicalls support. On Linux
targets, -mabicalls is the default, but bare-metal targets like
mips-sde-elf default to -mno-abicalls and naively compiling with -fPIC
alone doesn't result in working code. (In fact, even "-fPIC -mabicalls"
doesn't work, because -mabicalls then conflicts with the default for -G,
and the linker rejects mixing -mabicalls code with -mno-abicalls
libraries.)
Yeah. I was tempted to add a message for this a while back, but I think
there was resistance from someone who was managing to generate a form of
PIC with careful use of that combination. If they did, it was by accident
rather than design.
However, EABI doesn't support PIC at all. (There used to be an
-membedded-pic option, but that's long gone.) So I think this: [snip]
should be:
if (flag_pic)
{
if (mips_abi == ABI_EABI)
error ("cannot generate position-independent code for %qs",
"-mabi=eabi");
else if (!TARGET_ABICALLS)
error ("position-independent code requires %qs", "-mabicalls");
}
(where flag_pie implies flag_pic). I've removed the -fpic/-fpie thing
to avoid having to decide between printing "-fpic" and "-fPIC"
(or "-fpie" and "-fPIE").
OK with that change, if you agree.
Yes indeed -- I was dithering on exactly what the error message should
say anyway. I've committed the attached version.
-Sandra
2012-08-04 Sandra Loosemore
Richard Sandiford
gcc/
* config/mips/mips.c (mips_option_override): Check -fpic
for compatibility with -mabicalls and ABI.
gcc/testsuite/
* g++.dg/opt/enum2.C: Require fpic target.
* g++.dg/lto/20090303_0.C: Likewise.
Index: gcc/config/mips/mips.c
===
--- gcc/config/mips/mips.c (revision 190149)
+++ gcc/config/mips/mips.c (working copy)
@@ -16162,6 +16162,16 @@ mips_option_override (void)
target_flags &= ~MASK_ABICALLS;
}
+ /* PIC requires -mabicalls. */
+ if (flag_pic)
+{
+ if (mips_abi == ABI_EABI)
+ error ("cannot generate position-independent code for %qs",
+ "-mabi=eabi");
+ else if (!TARGET_ABICALLS)
+ error ("position-independent code requires %qs", "-mabicalls");
+}
+
if (TARGET_ABICALLS_PIC2)
/* We need to set flag_pic for executables as well as DSOs
because we may reference symbols that are not defined in
Index: gcc/testsuite/g++.dg/opt/enum2.C
===
--- gcc/testsuite/g++.dg/opt/enum2.C (revision 190149)
+++ gcc/testsuite/g++.dg/opt/enum2.C (working copy)
@@ -1,8 +1,8 @@
// PR c++/43680
// Test that we don't make excessively aggressive assumptions about what
// values an enum variable can have.
+// { dg-do run { target fpic } }
// { dg-options "-O2 -fPIC" }
-// { dg-do run }
extern "C" void abort ();
Index: gcc/testsuite/g++.dg/lto/20090303_0.C
===
--- gcc/testsuite/g++.dg/lto/20090303_0.C (revision 190149)
+++ gcc/testsuite/g++.dg/lto/20090303_0.C (working copy)
@@ -1,4 +1,5 @@
/* { dg-lto-do run } */
+/* { dg-require-effective-target fpic } */
/* { dg-lto-options {{-flto -flto-partition=1to1 -fPIC}} } */
/* { dg-lto-options {{-flto -flto-partition=1to1}} { target sparc*-*-* } } */
/* { dg-suppress-ld-options {-fPIC} } */
Re: [libiberty] add obstack macros (was Re: PR #53525 - track-macro-expansion performance regression)
On Sat, Aug 4, 2012 at 9:40 AM, Dimitrios Apostolou wrote: > On Fri, 3 Aug 2012, Ian Lance Taylor wrote: > >>> 2012-08-04 Dimitrios Apostolou >>> >>> * libiberty.h >>> (XOBDELETE,XOBGROW,XOBGROWVEC,XOBSHRINK,XOBSHRINKVEC): New >>> type-safe macros for obstack allocation. >>> (XOBFINISH): Renamed argument to PT since it is a pointer to T. >> >> >>> +/* Type-safe obstack allocator. You must first initialize the obstack >>> with >>> + obstack_init() or _obstack_begin(). >> >> >> This should recommend obstack_init, or obstack_begin, but not >> _obstack_begin. Also obstack_specify_allocation and >> obstack_specify_allocation_with_arg are OK, so really it might be >> better not to list the functions, but simply say "You must first >> initialization the obstack." > > > Grep reveals that 9 out of 10 times we use _obstack_begin(), and we set > alignment to 0 (isn't it suboptimal?). I'm not sure where you are looking. I only see one call to _obstack_begin in the gcc directory, and it could easily be replaced with a call to obstack_specify_allocation instead. It does set the alignment to 0, but that just directs the obstack code to use the default alignment, which is the alignment of double. I think that should normally be fine. >>> + T: Type, O: Obstack, N: Number of elements, S: raw Size, >> >> >> s/Size/size/ >> >>> +#define XOBSHRINK(O, T)obstack_blank ((O), -1 * sizeof >>> (T)) >>> +#define XOBSHRINKVEC(O, T, N) obstack_blank ((O), -1 * sizeof (T) * >>> (N)) >> >> >> These are hard to use safely. I'm not sure we should define them at all. > > > I've already used XOBSHRINK and it looks clear to me, but I could use > obstack_blank() directly if necessary. I'm a bit concerned because they only work if space has already been allocated, and there is no checking. Also I only see them used in genautomata.c. But I guess it's OK. >>> +#define XOBFINISH(O, PT) ((PT) obstack_finish ((O))) >> >> >> For XOBNEW, etc., we use (T *) rather than (PT). Using (PT) seems >> error-probe--it's the only use of the obstack with a different type >> parameter. Why not use T rather than PT here, and return (T *)? > > > I'd have to change many (about 60) occurences of XOBFINISH if I change that. > I'd go for it if I was sure it's what we want, it can be a separate patch > later on. I'm sorry to ask you to change a lot of code, but it simply doesn't make sense to me to have all but one macro take the type as an argument, and have one macro take a pointer to the type. They really have to be consistent. Ian
