Re: remove wrong code in immed_double_const
Mike Stump writes: > This removes some wrong code. > > Ok? > > Index: gcc/emit-rtl.c > === > --- gcc/emit-rtl.c (revision 184563) > +++ gcc/emit-rtl.c (working copy) > @@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO > >if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) > return gen_int_mode (i0, mode); > - > - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); > } > >/* If this integer fits in one word, return a CONST_INT. */ Is this because you have an integer mode between HOST_BITS_PER_WIDE_INT and 2 * HOST_BITS_PER_WIDE_INT? (I.e. a partial one?) If so, I can see an argument for changing the "==" to "<=", although we'd need to think carefully about what CONST_DOUBLE means in that case. (Endianness, etc.) Or is this because you have an integer mode wider than 2*OST_BITS_PER_WIDE_INT? We currently only support constant integer widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly triggering if we try to build a wider constant. Obviously it'd be nice if we supported arbitrary widths, e.g. by replacing CONST_INT and CONST_DOUBLE with a single n-HOST_WIDE_INT rtx (and immed_double_const with some kind of nary builder, etc.). It won't be easy though. Removing the assert seems like papering over the problem. FWIW, here's another case where this came up: http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html Richard
Re: [C++ Patch] PR 14710 (add -Wuseless-cast)
On 03/16/2012 05:42 PM, Paolo Carlini wrote: 3- References can be easily missed because wrapped in INDIRECT_REF: as explained at the beginning of tree.c and already used in many places, a REFERENCE_REF_P check (and in case a TREE_OPERAND (expr, 0)) takes care of that. I'm not 100% sure the solution is fully general, though. That won't catch something like int i; static_cast(i); which is also a useless cast, because i is already an int lvalue; not all lvalues are derived from references. Note that something like static_cast(42); is not a useless cast, because it turns a prvalue into an xvalue. Jason
[Patch, libfortran] PR 52608 F formatting with scale factor
Hi,
a recent patch by yours truly caused incorrect output for the
combination of scale factor, the value containing initial zeroes, and
F editing. Fixed by moving the removal of the initial zeros until
after the scale factor has been applied to the value. Committed the
patch below as obvious after regtesting and running the NIST
testsuite.
Index: ChangeLog
===
--- ChangeLog (revision 185485)
+++ ChangeLog (working copy)
@@ -1,3 +1,9 @@
+2012-03-17 Janne Blomqvist
+
+ PR libfortran/52608
+ * io/write_float.def (output_float): Move removal of initial zeros
+ until after the scale factor has been applied.
+
2012-03-16 Janne Blomqvist
* io/unix.h (struct stream): Rename to stream_vtable.
Index: io/write_float.def
===
--- io/write_float.def (revision 185485)
+++ io/write_float.def (working copy)
@@ -180,12 +180,6 @@ output_float (st_parameter_dt *dtp, cons
/* Make sure the decimal point is a '.'; depending on the
locale, this might not be the case otherwise. */
digits[nbefore] = '.';
- if (digits[0] == '0' && nbefore == 1)
- {
- digits++;
- nbefore--;
- ndigits--;
- }
if (p != 0)
{
if (p > 0)
@@ -229,6 +223,13 @@ output_float (st_parameter_dt *dtp, cons
nafter = d;
}
+ while (digits[0] == '0' && nbefore > 0)
+ {
+ digits++;
+ nbefore--;
+ ndigits--;
+ }
+
expchar = 0;
/* If we need to do rounding ourselves, get rid of the dot by
moving the fractional part. */
--
Janne Blomqvist
[PATCH, i386] Remove empty predicates and/or constraints.
On Sat, Mar 17, 2012 at 5:09 AM, Hans-Peter Nilsson wrote:
>> A small no-op change - there is no need for a constraint in an expand
>> pattern. Plus some formatting.
>
> If you want to remove it, then remove it, don't just empty it. ;)
Something like attached is probably even better ;)
2012-03-17 Uros Bizjak
* config/i386/i386.md: Remove empty predicates and/or constraints.
* config/i386/sync.md: Ditto.
* config/i386/sse.md: Ditto.
* config/i386/mmx.md: Ditto.
* config/i386/pentium.md: Ditto.
* config/i386/athlon.md: Ditto.
Bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32}. If
there are no objections, I'll commit the patch tomorrow.
Uros.
p.diff.txt.bz2
Description: BZip2 compressed data
Re: PATCH: Properly generate X32 IE sequence
On Tue, Mar 13, 2012 at 3:37 AM, Uros Bizjak wrote:
> On Tue, Mar 13, 2012 at 8:11 AM, Uros Bizjak wrote:
>
> Please try attached patch. It introduces TARGET_TLS_INDIRECT_SEG_REFS
> to block only indirect seg references.
>>>
>>> There is no regression.
>>
>> Thanks, committed to mainline SVN with following ChangeLog:
>>
>> 2012-03-13 Uros Bizjak
>>
>> * config/i386/i386.h (TARGET_TLS_INDIRECT_SEG_REFS): New.
>> * config/i386/i386.c (ix86_decompose_address): Use
>> TARGET_TLS_INDIRECT_SEG_REFS to prevent %fs:(%reg) addresses.
>> (legitimize_tls_address): Use TARGET_TLS_INDIRECT_SEG_REFS to load
>> thread pointer to a register.
>>
>> Tested on x86_64-pc-linux-gnu {,-m32}.
>>
>>> BTW, this x32 TLS IE optimization:
>>
>> > movq %rax, %fs:(%rdx)
>>
>> This is just looking for troubles. If we said these addresses are
>> invalid, then we shouldn't generate them.
>
> OTOH, we can improve rejection test a bit to reject only non-word
> mode registers.
>
> 2012-03-13 Uros Bizjak
>
> * config/i386/i386.c (ix86_decompose_address): Prevent %fs:(%reg)
> addresses only when %reg is not in word mode.
>
> Tested on x86_64-pc-linux-gnu {,-m32}, committed.
>
> Uros.
>
> Index: i386.c
> ===
> --- i386.c (revision 185278)
> +++ i386.c (working copy)
> @@ -11563,8 +11563,10 @@
> return 0;
> }
>
> - if (seg != SEG_DEFAULT && (base || index)
> - && !TARGET_TLS_INDIRECT_SEG_REFS)
> +/* Address override works only on the (%reg) part of %fs:(%reg). */
> + if (seg != SEG_DEFAULT
> + && ((base && GET_MODE (base) != word_mode)
> + || (index && GET_MODE (index) != word_mode)))
> return 0;
>
> /* Extract the integral value of scale. */
Is my x32 TLS IE optimization:
http://gcc.gnu.org/ml/gcc-patches/2012-03/msg00714.html
OK for trunk?
Thanks.
--
H.J.
Re: PATCH: Properly generate X32 IE sequence
On Sun, Mar 11, 2012 at 6:11 PM, H.J. Lu wrote: > Since we must use reg64 in %fs:(%reg) memory operand like > > movq x@gottpoff(%rip),%reg64; > mov %fs:(%reg64),%reg > > this patch optimizes x32 TLS IE load and store by wrapping > %reg64 inside of UNSPEC when Pmode == SImode. OK for > trunk? > > Thanks. > > -- > H.J. > --- > 2012-03-11 H.J. Lu > > * config/i386/i386.md (*tls_initial_exec_x32_load): New. > (*tls_initial_exec_x32_store): Likewise. Can you implement this with define_insn_and_split, like i.e. *tls_dynamic_gnu2_combine_32 ? Uros.
Re: PATCH: Properly generate X32 IE sequence
On Sat, Mar 17, 2012 at 11:10 AM, Uros Bizjak wrote: > On Sun, Mar 11, 2012 at 6:11 PM, H.J. Lu wrote: > >> Since we must use reg64 in %fs:(%reg) memory operand like >> >> movq x@gottpoff(%rip),%reg64; >> mov %fs:(%reg64),%reg >> >> this patch optimizes x32 TLS IE load and store by wrapping >> %reg64 inside of UNSPEC when Pmode == SImode. OK for >> trunk? >> >> Thanks. >> >> -- >> H.J. >> --- >> 2012-03-11 H.J. Lu >> >> * config/i386/i386.md (*tls_initial_exec_x32_load): New. >> (*tls_initial_exec_x32_store): Likewise. > > Can you implement this with define_insn_and_split, like i.e. > *tls_dynamic_gnu2_combine_32 ? > I will give it a try again. Last time when I tried it, GCC didn't like memory operand in DImode when Pmode == SImode. -- H.J.
Re: PATCH: Properly generate X32 IE sequence
On Sat, Mar 17, 2012 at 7:18 PM, H.J. Lu wrote: >>> Since we must use reg64 in %fs:(%reg) memory operand like >>> >>> movq x@gottpoff(%rip),%reg64; >>> mov %fs:(%reg64),%reg >>> >>> this patch optimizes x32 TLS IE load and store by wrapping >>> %reg64 inside of UNSPEC when Pmode == SImode. OK for >>> trunk? >>> >>> Thanks. >>> >>> -- >>> H.J. >>> --- >>> 2012-03-11 H.J. Lu >>> >>> * config/i386/i386.md (*tls_initial_exec_x32_load): New. >>> (*tls_initial_exec_x32_store): Likewise. >> >> Can you implement this with define_insn_and_split, like i.e. >> *tls_dynamic_gnu2_combine_32 ? >> > > I will give it a try again. Last time when I tried it, GCC didn't > like memory operand in DImode when Pmode == SImode. You should remove mode for tls_symbolic_operand predicate. Uros.
Re: [Patch, libfortran] PR 52608 F formatting with scale factor
On Sat, Mar 17, 2012 at 19:23, Janne Blomqvist
wrote:
> Hi,
>
> a recent patch by yours truly caused incorrect output for the
> combination of scale factor, the value containing initial zeroes, and
> F editing. Fixed by moving the removal of the initial zeros until
> after the scale factor has been applied to the value. Committed the
> patch below as obvious after regtesting and running the NIST
> testsuite.
And upon request, a testcase reduced from the NIST testsuite, which
caught the bug in the first place:
2012-03-17 Janne Blomqvist
PR libfortran/52608
* gfortran.dh/pr52608.f90: New test.
! { dg-do run }
! PR 52608
! Testcase reduced from NIST testsuite FM110
program fm110_snippet
implicit none
real :: aavs
character(len=100) :: s(2), s2(2)
AAVS = .087654
35043 FORMAT (" ",16X,"COMPUTED: ",22X,1P/26X,F5.4,3X,2P,F5.3,+3P," ",&
(23X,F6.2),3X)
5043 FORMAT (17X,"CORRECT: ",/24X,&
" .8765 8.765 87.65")
WRITE (s,35043) AAVS,AAVS,AAVS
WRITE (s2,5043)
if (s(2) /= s2(2)) call abort()
end program fm110_snippet
Committed as obvious.
--
Janne Blomqvist
[fortran, patch] Follow-up "widechar error" patch
This patch fixes PR 52559 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52559), which was due to my earlier patch for displaying error loci in lines containing wide characters (http://gcc.gnu.org/ml/fortran/2012-03/msg00015.html). In preexisting code, a tab is displayed as a single space when an error is printed. I didn't handle it consistently, which should now be fixed. Bootstrapped and regtested on x86_64-apple-darwin11. OK to commit? FX errorfix.ChangeLog Description: Binary data errorfix.diff Description: Binary data
Re: PATCH: Properly generate X32 IE sequence
On Sat, Mar 17, 2012 at 11:20 AM, Uros Bizjak wrote:
> On Sat, Mar 17, 2012 at 7:18 PM, H.J. Lu wrote:
>
Since we must use reg64 in %fs:(%reg) memory operand like
movq x@gottpoff(%rip),%reg64;
mov %fs:(%reg64),%reg
this patch optimizes x32 TLS IE load and store by wrapping
%reg64 inside of UNSPEC when Pmode == SImode. OK for
trunk?
Thanks.
--
H.J.
---
2012-03-11 H.J. Lu
* config/i386/i386.md (*tls_initial_exec_x32_load): New.
(*tls_initial_exec_x32_store): Likewise.
>>>
>>> Can you implement this with define_insn_and_split, like i.e.
>>> *tls_dynamic_gnu2_combine_32 ?
>>>
>>
>> I will give it a try again. Last time when I tried it, GCC didn't
>> like memory operand in DImode when Pmode == SImode.
>
> You should remove mode for tls_symbolic_operand predicate.
>
I am testing this patch. OK for trunk if it passes all tests?
Thanks.
--
H.J.
2012-03-17 H.J. Lu
* config/i386/i386-protos.h (ix86_split_tls_initial_exec_x32): New.
* config/i386/i386.c (ix86_split_tls_initial_exec_x32): Likewise.
* config/i386/i386.md (*tls_initial_exec_x32_load): New.
(*tls_initial_exec_x32_store): Likewise.
diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h
index 630112f..2c4f1ed 100644
--- a/gcc/config/i386/i386-protos.h
+++ b/gcc/config/i386/i386-protos.h
@@ -213,6 +213,7 @@ extern unsigned int ix86_get_callcvt (const_tree);
#endif
extern rtx ix86_tls_module_base (void);
+extern void ix86_split_tls_initial_exec_x32 (rtx [], enum machine_mode, bool);
extern void ix86_expand_vector_init (bool, rtx, rtx);
extern void ix86_expand_vector_set (bool, rtx, rtx, int);
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 78a366e..5a9c673 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -12754,6 +12754,28 @@ legitimize_tls_address (rtx x, enum tls_model model,
bool for_mov)
return dest;
}
+/* Split x32 TLS IE access in MODE. Split load if LOAD is TRUE,
+ otherwise split store. */
+
+void
+ix86_split_tls_initial_exec_x32 (rtx operands[],
+enum machine_mode mode, bool load)
+{
+ rtx base, mem;
+ rtx off = load ? operands[1] : operands[0];
+ off = gen_rtx_UNSPEC (DImode, gen_rtvec (1, off), UNSPEC_GOTNTPOFF);
+ off = gen_rtx_CONST (DImode, off);
+ off = gen_const_mem (DImode, off);
+ set_mem_alias_set (off, ix86_GOT_alias_set ());
+ base = gen_rtx_UNSPEC (DImode, gen_rtvec (1, const0_rtx), UNSPEC_TP);
+ off = gen_rtx_PLUS (DImode, base, force_reg (DImode, off));
+ mem = gen_rtx_MEM (mode, off);
+ if (load)
+emit_move_insn (operands[0], mem);
+ else
+emit_move_insn (mem, operands[1]);
+}
+
/* Create or return the unique __imp_DECL dllimport symbol corresponding
to symbol DECL. */
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index eae26ae..78faeec 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -12858,6 +12858,32 @@
}
[(set_attr "type" "multi")])
+(define_insn_and_split "*tls_initial_exec_x32_load"
+ [(set (match_operand:SWI1248x 0 "register_operand" "=r")
+(mem:SWI1248x
+ (unspec:SI
+ [(match_operand 1 "tls_symbolic_operand" "")]
+ UNSPEC_TLS_IE_X32)))
+ (clobber (reg:CC FLAGS_REG))]
+ "TARGET_X32"
+ "#"
+ ""
+ [(const_int 0)]
+ "ix86_split_tls_initial_exec_x32 (operands, mode, TRUE); DONE;")
+
+(define_insn_and_split "*tls_initial_exec_x32_store"
+ [(set (mem:SWI1248x
+ (unspec:SI
+ [(match_operand 0 "tls_symbolic_operand" "")]
+ UNSPEC_TLS_IE_X32))
+ (match_operand:SWI1248x 1 "register_operand" "r"))
+ (clobber (reg:CC FLAGS_REG))]
+ "TARGET_X32"
+ "#"
+ ""
+ [(const_int 0)]
+ "ix86_split_tls_initial_exec_x32 (operands, mode, FALSE); DONE;")
+
;; GNU2 TLS patterns can be split.
(define_expand "tls_dynamic_gnu2_32"
Re: remove wrong code in immed_double_const
On Mar 17, 2012, at 12:37 AM, Richard Sandiford wrote: > Mike Stump writes: >> This removes some wrong code. >> >> Ok? >> >> Index: gcc/emit-rtl.c >> === >> --- gcc/emit-rtl.c (revision 184563) >> +++ gcc/emit-rtl.c (working copy) >> @@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO >> >> if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT) >>return gen_int_mode (i0, mode); >> - >> - gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT); >> } >> >> /* If this integer fits in one word, return a CONST_INT. */ > > Is this because you have an integer mode between HOST_BITS_PER_WIDE_INT and > 2 * HOST_BITS_PER_WIDE_INT? Yes, I have those, but, that wasn't the testcase I had in mind. > Or is this because you have an integer mode wider than > 2*OST_BITS_PER_WIDE_INT? Yes. > We currently only support constant integer > widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly > triggering if we try to build a wider constant. Can you give me a concrete example of what will fail with the constant 0 generated by: return GEN_INT (i0); when the mode is 2*HOST_BITS_PER_WIDE_INT? When the mode is larger than this? If you cannot, can you refer me to documentation where this is discussed? If not, how about code? What I am seeing is that it works and generates correct code without the assert. My contention is that any code that fails to work, is buggy, and should be fixed, and once fixed, then there is no code that doesn't work, and hence the assert is wrong. If you want to argue that the code that fails, isn't buggy, go ahead, I'd love to hear it. See, we already shorten the width of wide constants and expect that to work. This is just another instance of shortening. Now, just to see what lurks in there, I when through 100% of the uses of CONST_DOUBLE in *.c to get a feel for what might go wrong, the code is as benign as I expected, every instance I saw, looked reasonably safe. It doesn't get any better than this. > Obviously it'd be nice Yes, but that is quite a lot of work. It also happens to be orthogonal to the immediate issue at hand. > if we supported arbitrary widths, e.g. by replacing CONST_INT and > CONST_DOUBLE with a single n-HOST_WIDE_INT rtx (and immed_double_const > with some kind of nary builder, etc.). It won't be easy though. > > Removing the assert seems like papering over the problem. Do you have an example of a failure? Hint, without a failure, there is no bug, I cannot fix a bug, when there is no bug. > FWIW, here's another case where this came up: > >http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html Yes, and surprisingly, or not it is the exact same case as I can into. Notice nowhere in that bug or thread, is _any_ detailed discussed as to why that would be a bad fix. So, I'm looking for approval, or a concrete reason why it is a bad idea.
Re: [google][4.6]Make option -freorder-functions= invoke function reordering linker plugin (issue 5825054)
Ok for google branches after updating the doc/invoke.texi file. David http://codereview.appspot.com/5825054/
