Re: [committed] Get avr building again

[Senthil Kumar Selvaraj via Gcc-patches]

Jeff Law via Gcc-patches writes:

> Removes references to CC_STATUS_INIT from the avr port, which should get 
> it to the point of building again.
>
>
> Committed to the trunk.

Thanks, I was about to send a patch for that.

Regards
Senthil
>
>
> Jeff

Re: [Ping, PATCH 2/n] AVR CC0 conversion - adjust peepholes

[Senthil Kumar Selvaraj via Gcc-patches]

Could someone please approve this patch too? The base conversion patch
(https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568658.html) was
approved and committed, and fixing peepholes addresses a bunch of code
size regressions introduced by the base patch.

No regressions on all 4 devices, as mentioned in the base patch
submission.

Regards
Senthil

Senthil Kumar Selvaraj writes:

> Applies cleanly on rebased version of previous patch
> (https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568658.html).
>
> Senthil Kumar Selvaraj writes:
>
>> Hi,
>>
>> This patch, to be applied on top of
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563638.html,
>> adjusts peepholes to match and generate parallels with a clobber of
>> REG_CC.
>>
>> It also sets mov_insn as the name of the pattern for the split
>> insn (rather than the define_insn_and_split), so that
>> avr_2word_insn_p, which looks for CODE_FOR_mov_insn, works
>> correctly. This is required for the *cpse.eq peephole to fire, and
>> also helps generate better code for avr_out_sbxx_branch.
>>
>> There are no regressions, and the number of test cases reporting
>> UNSUPPORTED or FAIL because of code size changes (when compared to
>> mainline) for attiny40 and atmega8 are now down to 3 and 3,
>> respectively, from 10 and 25 previously.
>>
>> The embench-iot numbers also show a good improvement.
>>
>> Benchmark   Baseline  Current   Increase %
>> - ---   --
>> aha-mont64  6,944   6,944   0
>> crc32 704 706   0.28
>> cubic   9,428   9,428   0
>> edn 3,854   3,854   0
>> huffbench   2,890   2,890   0
>> matmult-int 1,164   1,164   0
>> minver  3,960   3,956  -0.1
>> nbody   3,106   3,110   0.13
>> nettle-aes  5,292   5,304   0.23
>> nettle-sha256  25,748  25,748   0
>> nsichneu   39,622  39,622   0
>> picojpeg9,898   9,980   0.83
>> qrduino 9,234   9,356   1.32
>> sglib-combined  4,658   4,658   0
>> slre4,000   4,000   0
>> st  3,356   3,356   0
>> statemate   5,490   5,502   0.22
>> ud  2,940   2,940   0
>> wikisort   20,776  20,772   -0.02
>>
>> Regards
>> Senthil
>>
>>
>> gcc/ChangeLog:
>>
>>  * config/avr/avr.md: Adjust peepholes to match and
>>  generate parallels with clobber of REG_CC.
>>  (mov_insn): Rename to mov_insn_split.
>>  (*mov_insn): Rename to mov_insn.
>>
>>
>> diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
>> index 2206fa19671..a1a325b7a8c 100644
>> --- gcc/config/avr/avr.md
>> +++ gcc/config/avr/avr.md
>> @@ -724,9 +724,7 @@ (define_expand "mov"
>>  ;; are call-saved registers, and most of LD_REGS are call-used registers,
>>  ;; so this may still be a win for registers live across function calls.
>>
>> -;; "movqi_insn"
>> -;; "movqq_insn" "movuqq_insn"
>> -(define_insn_and_split "mov_insn"
>> +(define_insn_and_split "mov_insn_split"
>>[(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
>> ,q,r,*r")
>>  (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
>> Y00,Qm,r,q,i"))]
>>"register_operand (operands[0], mode)
>> @@ -737,7 +735,9 @@ (define_insn_and_split "mov_insn"
>> (match_dup 1))
>>(clobber (reg:CC REG_CC))])])
>>
>> -(define_insn "*mov_insn"
>> +;; "movqi_insn"
>> +;; "movqq_insn" "movuqq_insn"
>> +(define_insn "mov_insn"
>>[(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
>> ,q,r,*r")
>>  (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
>> Y00,Qm,r,q,i"))
>> (clobber (reg:CC REG_CC))]
>> @@ -758,7 +758,8 @@ (define_insn "*mov_insn"
>>  (define_insn "*reload_in"
>>[(set (match_operand:ALL1 0 "register_operand""=l")
>>  (match_operand:ALL1 1 "const_operand""i"))
>> -   (clobber (match_operand:QI 2 "register_operand" "="))]
>> +   (clobber (match_operand:QI 2 "reg

Re: [Ping] AVR CC0 conversion

[Senthil Kumar Selvaraj via Gcc-patches]

John Paul Adrian Glaubitz writes:

> On 4/28/21 7:59 PM, Senthil Kumar Selvaraj wrote:
>>>> OK for trunk.
>>>
>>> Anything else that keeps us from merging the changes? Would be great to
>>> have the last backend besides CR-16 finally converted to MODE_CC on trunk.
>>
>> Nope. Committed and pushed just now  -
>> https://gcc.gnu.org/git?p=gcc.git;a=commit;h=3ba781d3b5c8efadb60866c9743b657e8f0eb222
>
> Awesome \o/. Should we wait for the second patch [1] to be merged as well
> before closing the PR? [2]

I guess it depends on the scope of the PR? If it was about removing cc0,
then this patch would do. If it was about getting the generated code
also to be as close to what it was with cc0, then no, it cannot be closed.

OTOH, while the second patch (adjusting peepholes) does help, there are
other things in the pipeline - addition of new CC modes and enabling
compare elimination, adjusting rtx costs etc. I will continue to work on
them in my spare time and submit patches when ready.

Regards
Senthil

>
> Adrian
>
>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568659.html
>> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92729

Re: [Ping] AVR CC0 conversion

[Senthil Kumar Selvaraj via Gcc-patches]

John Paul Adrian Glaubitz writes:

> Hi Senthil!
>
>> On Mon, Apr 26, 2021 at 9:20 AM Senthil Kumar Selvaraj via Gcc-patches
>>  wrote:
>>>
>>> Hi,
>>>
>>> This is
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563638.html,
>>> rebased against latest gcc master. The only change is modification of
>>> avr_md_asm_adjust's signature to match the extra input_modes parameter
>>> that TARGET_MD_ASM_ADJUST gained.
>>>
>>> I re-ran regression tests for atmega128, atxmega128a3, attiny40 and
>>> atmega8 (after applying
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563779.html
>>> also) and verified that the results matched the original patchset.
>> 
>> OK for trunk.
>
> Anything else that keeps us from merging the changes? Would be great to
> have the last backend besides CR-16 finally converted to MODE_CC on trunk.

Nope. Committed and pushed just now  -
https://gcc.gnu.org/git?p=gcc.git;a=commit;h=3ba781d3b5c8efadb60866c9743b657e8f0eb222

Regards
Senthil
>
> Adrian

[Ping, PATCH 2/n] AVR CC0 conversion - adjust peepholes

[Senthil Kumar Selvaraj via Gcc-patches]

Applies cleanly on rebased version of previous patch
(https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568658.html).

Senthil Kumar Selvaraj writes:

> Hi,
>
> This patch, to be applied on top of
> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563638.html,
> adjusts peepholes to match and generate parallels with a clobber of
> REG_CC.
>
> It also sets mov_insn as the name of the pattern for the split
> insn (rather than the define_insn_and_split), so that
> avr_2word_insn_p, which looks for CODE_FOR_mov_insn, works
> correctly. This is required for the *cpse.eq peephole to fire, and
> also helps generate better code for avr_out_sbxx_branch.
>
> There are no regressions, and the number of test cases reporting
> UNSUPPORTED or FAIL because of code size changes (when compared to
> mainline) for attiny40 and atmega8 are now down to 3 and 3,
> respectively, from 10 and 25 previously.
>
> The embench-iot numbers also show a good improvement.
>
> Benchmark   Baseline  Current   Increase %
> - ---   --
> aha-mont64  6,944   6,944   0
> crc32 704 706   0.28
> cubic   9,428   9,428   0
> edn 3,854   3,854   0
> huffbench   2,890   2,890   0
> matmult-int 1,164   1,164   0
> minver  3,960   3,956  -0.1
> nbody   3,106   3,110   0.13
> nettle-aes  5,292   5,304   0.23
> nettle-sha256  25,748  25,748   0
> nsichneu   39,622  39,622   0
> picojpeg9,898   9,980   0.83
> qrduino 9,234   9,356   1.32
> sglib-combined  4,658   4,658   0
> slre4,000   4,000   0
> st  3,356   3,356   0
> statemate   5,490   5,502   0.22
> ud  2,940   2,940   0
> wikisort   20,776  20,772   -0.02
>
> Regards
> Senthil
>
>
> gcc/ChangeLog:
>
>   * config/avr/avr.md: Adjust peepholes to match and
>   generate parallels with clobber of REG_CC.
>   (mov_insn): Rename to mov_insn_split.
>   (*mov_insn): Rename to mov_insn.
>
>
> diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
> index 2206fa19671..a1a325b7a8c 100644
> --- gcc/config/avr/avr.md
> +++ gcc/config/avr/avr.md
> @@ -724,9 +724,7 @@ (define_expand "mov"
>  ;; are call-saved registers, and most of LD_REGS are call-used registers,
>  ;; so this may still be a win for registers live across function calls.
>  
> -;; "movqi_insn"
> -;; "movqq_insn" "movuqq_insn"
> -(define_insn_and_split "mov_insn"
> +(define_insn_and_split "mov_insn_split"
>[(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
> ,q,r,*r")
>  (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
> Y00,Qm,r,q,i"))]
>"register_operand (operands[0], mode)
> @@ -737,7 +735,9 @@ (define_insn_and_split "mov_insn"
> (match_dup 1))
>(clobber (reg:CC REG_CC))])])
>  
> -(define_insn "*mov_insn"
> +;; "movqi_insn"
> +;; "movqq_insn" "movuqq_insn"
> +(define_insn "mov_insn"
>[(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
> ,q,r,*r")
>  (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
> Y00,Qm,r,q,i"))
> (clobber (reg:CC REG_CC))]
> @@ -758,7 +758,8 @@ (define_insn "*mov_insn"
>  (define_insn "*reload_in"
>[(set (match_operand:ALL1 0 "register_operand""=l")
>  (match_operand:ALL1 1 "const_operand""i"))
> -   (clobber (match_operand:QI 2 "register_operand" "="))]
> +   (clobber (match_operand:QI 2 "register_operand" "="))
> +   (clobber (reg:CC REG_CC))]
>"reload_completed"
>"ldi %2,lo8(%1)
>   mov %0,%2"
> @@ -766,15 +767,17 @@ (define_insn "*reload_in"
>  
>  (define_peephole2
>[(match_scratch:QI 2 "d")
> -   (set (match_operand:ALL1 0 "l_register_operand" "")
> -(match_operand:ALL1 1 "const_operand" ""))]
> +   (parallel [(set (match_operand:ALL1 0 "l_register_operand" "")
> +   (match_operand:ALL1 1 "const_operand" ""))
> +  (clobber (reg:CC REG_CC))])]
>; No need for a clobber reg for 0x0, 0x01 or 0xff
>"!satisfie

[PATCH 2/n] AVR CC0 conversion - adjust peepholes

[Senthil Kumar Selvaraj via Gcc-patches]

Hi,

This patch, to be applied on top of
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563638.html,
adjusts peepholes to match and generate parallels with a clobber of
REG_CC.

It also sets mov_insn as the name of the pattern for the split
insn (rather than the define_insn_and_split), so that
avr_2word_insn_p, which looks for CODE_FOR_mov_insn, works
correctly. This is required for the *cpse.eq peephole to fire, and
also helps generate better code for avr_out_sbxx_branch.

There are no regressions, and the number of test cases reporting
UNSUPPORTED or FAIL because of code size changes (when compared to
mainline) for attiny40 and atmega8 are now down to 3 and 3,
respectively, from 10 and 25 previously.

The embench-iot numbers also show a good improvement.

Benchmark   Baseline  Current   Increase %
- ---   --
aha-mont64  6,944   6,944   0
crc32 704 706   0.28
cubic   9,428   9,428   0
edn 3,854   3,854   0
huffbench   2,890   2,890   0
matmult-int 1,164   1,164   0
minver  3,960   3,956  -0.1
nbody   3,106   3,110   0.13
nettle-aes  5,292   5,304   0.23
nettle-sha256  25,748  25,748   0
nsichneu   39,622  39,622   0
picojpeg9,898   9,980   0.83
qrduino 9,234   9,356   1.32
sglib-combined  4,658   4,658   0
slre4,000   4,000   0
st  3,356   3,356   0
statemate   5,490   5,502   0.22
ud  2,940   2,940   0
wikisort   20,776  20,772   -0.02

Regards
Senthil


gcc/ChangeLog:

* config/avr/avr.md: Adjust peepholes to match and
generate parallels with clobber of REG_CC.
(mov_insn): Rename to mov_insn_split.
(*mov_insn): Rename to mov_insn.


diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
index 2206fa19671..a1a325b7a8c 100644
--- gcc/config/avr/avr.md
+++ gcc/config/avr/avr.md
@@ -724,9 +724,7 @@ (define_expand "mov"
 ;; are call-saved registers, and most of LD_REGS are call-used registers,
 ;; so this may still be a win for registers live across function calls.
 
-;; "movqi_insn"
-;; "movqq_insn" "movuqq_insn"
-(define_insn_and_split "mov_insn"
+(define_insn_and_split "mov_insn_split"
   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
,q,r,*r")
 (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
Y00,Qm,r,q,i"))]
   "register_operand (operands[0], mode)
@@ -737,7 +735,9 @@ (define_insn_and_split "mov_insn"
(match_dup 1))
   (clobber (reg:CC REG_CC))])])
 
-(define_insn "*mov_insn"
+;; "movqi_insn"
+;; "movqq_insn" "movuqq_insn"
+(define_insn "mov_insn"
   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
,q,r,*r")
 (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
Y00,Qm,r,q,i"))
(clobber (reg:CC REG_CC))]
@@ -758,7 +758,8 @@ (define_insn "*mov_insn"
 (define_insn "*reload_in"
   [(set (match_operand:ALL1 0 "register_operand""=l")
 (match_operand:ALL1 1 "const_operand""i"))
-   (clobber (match_operand:QI 2 "register_operand" "="))]
+   (clobber (match_operand:QI 2 "register_operand" "="))
+   (clobber (reg:CC REG_CC))]
   "reload_completed"
   "ldi %2,lo8(%1)
mov %0,%2"
@@ -766,15 +767,17 @@ (define_insn "*reload_in"
 
 (define_peephole2
   [(match_scratch:QI 2 "d")
-   (set (match_operand:ALL1 0 "l_register_operand" "")
-(match_operand:ALL1 1 "const_operand" ""))]
+   (parallel [(set (match_operand:ALL1 0 "l_register_operand" "")
+   (match_operand:ALL1 1 "const_operand" ""))
+  (clobber (reg:CC REG_CC))])]
   ; No need for a clobber reg for 0x0, 0x01 or 0xff
   "!satisfies_constraint_Y00 (operands[1])
&& !satisfies_constraint_Y01 (operands[1])
&& !satisfies_constraint_Ym1 (operands[1])"
   [(parallel [(set (match_dup 0)
(match_dup 1))
-  (clobber (match_dup 2))])])
+  (clobber (match_dup 2))
+  (clobber (reg:CC REG_CC))])])
 
 ;;
 ;; move word (16 bit)
@@ -804,12 +807,14 @@ (define_insn "movhi_sp_r"
 
 (define_peephole2
   [(match_scratch:QI 2 "d")
-   (set (match_operand:ALL2 0 "l_register_operand" "")
-(match_operand:ALL2 1 "const_or_immediate_operand" ""))]
+   (parallel [(set (match_operand:ALL2 0 "l_register_operand" "")
+   (match_operand:ALL2 1 "const_or_immediate_operand" ""))
+  (clobber (reg:CC REG_CC))])]
   "operands[1] != CONST0_RTX (mode)"
   [(parallel [(set (match_dup 0)
(match_dup 1))
-  (clobber (match_dup 2))])])
+  (clobber (match_dup 2))
+  (clobber (reg:CC REG_CC))])])
 
 ;; '*' because it is not used in 

Re: [PATCH] avr: cc0 to mode_cc conversion

[Senthil Kumar Selvaraj via Gcc-patches]

Senthil Kumar Selvaraj writes:

> Georg-Johann Lay writes:
>
>>
>> Finally, some general remarks:
>
> The work on my github branch was not complete - I'd blindly followed
> whatever the CC0 Transition wiki mentioned (the first three steps of
> case #2), and fixed any regression fallout (for ATmega128).
>
> I intend to try out a define_subst/early clobber of reg_cc based
> approach (inspired by the cris port) and see if that can help avoid the
> proliferation of define_insn_and_splits. Will update how that works out.

I had some time this past week to try implementing some of the changes
you suggested.

>
>>
>> 2) We just saw 100reds of insns being dublicated, basically the whole
>> machine description except for the few insns that leave cc alone.
>> Isn't is possible to use define subst for the bulk of the insns and
>> get a neat code that's better to grasp and to maintain?
>> After all it's just appending a clobber of reg_cc, and in the current
>> proposal almost 50% of the backend is just redundent repetitions of
>> previous insns.

I could not find a way to get define_subst to do define_insn_and_split -
other targets using the same approach (pdp11, h8300) have the
duplication as well.

>>
>> 4) Many insns don't have reloads and don't need to be turned into a
>> splitter + yet another insns, it should be all right to clobber
>> reg_cc from the very start.  Or am I missing something?  I think
>> I marked all places, but it should be easy enough to spot them.

If I remove the define_insn_and_split and add a (clobber (reg:CC
REG_CC)) to the define_insn itself for xcall patterns, then the producer
of the pattern (define_expand, output template of
define-insn-and-split/define-split etc.. or C code) needs to modified to
include the clobber of REG_CC in a PARALLEL, so that's a whole bunch of
changes.

If that is done at define_expand, for example, then similar patterns
that do not use the hard regs (non-call variants) will also need to be
modified to add the clobber, and therefore there's no point in
define_insn without clobber and split after reload with clobber for
those patterns.

Did I get that right?

FWIW, I'm also working on a parallel implementation that clobbers REG_CC
in all patterns from the start (with matching clobbers in define_expand
etc..) - still not in good enough shape though. It will avoid
duplication, but at the expense of modification of nearly every pattern
to emit or accept a clobber of REG_CC.

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj via Gcc]

Pip Cet writes:

> On Tue, Aug 18, 2020 at 6:52 AM Senthil Kumar Selvaraj
>  wrote:
>> > recognize such insns, but as it stands that define_insn would
>> > recognize the incorrect insn:
>> >
>> > [(set (reg:QI 0) (const_int 0))
>> >  (clobber (scratch:CC))]
>>
>> get_cc_reg_clobber_rtx also looks at the insn itself (i.e. whatever
>> the erstwhile NOTICE_UPDATE_CC hook did), so if the cc attribute is LDI,
>> and operands[1] is const0_rtx, it would return cc_reg_rtx as the clobber reg.
>>
>> AFAICT, some passes after reload verify if operands match constraints,
>> and that would work in this case - I'm not sure if the pattern you
>> mentioned could show up, outside of wrong splitters/peepholes in the md file.
>
> I don't think they can, but it's still another case of lying to GCC.
>
> At the very least, output_movqi should assert that it isn't asked to
> produce code for this situation.

I agree.
>
>> Another approach would be to conservatively use a 'c' constraint and
>> clobber REG_CC for all reg-reg moves.
>
> I'd prefer splitting the constraint alternatives to have one clear-r0
> alternative, an ldi alternative, and a clear -r1_31 alternative.
>
> As for define_subst, is it really worth it? If I'm reading the
> documentation correctly, it's not powerful enough to deal with scratch
> operands on its own, so we'd probably need three or four variants of
> define_subst just to handle those cases.

Is this related to cmpelim not looking at REG_CC clobber if there are
other (clobber scratch..) preceding it?
>
> I'm probably missing something obvious, but what's the reason for
> keeping the CC-clobbering post-reload splitter when we already have a
> CC-setting one? Are there significant post-reload optimizations that
> can deal with a clobber but not an arbitrary assignment to CC? If so,
> wouldn't it be better to fix those optimizations?

The post-reload splitter introduces the clobber. The wiki
suggests that approach if most insns clobber REG_CC, perhaps because of
the missed optimizations you describe below?
>
> (There are at least some pre-reload optimizations that don't work with
> the CC-clobbering insn patterns. lower_subreg.c's simple_move, for
> example, recognizes only two-operand sets. This resulted in pessimized
> code, but is easy to fix.)

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj via Gcc]

Hans-Peter Nilsson writes:

> On Wed, 19 Aug 2020, Senthil Kumar Selvaraj wrote:
>>
>> Hans-Peter Nilsson writes:
>>
>> > On Fri, 14 Aug 2020, Senthil Kumar Selvaraj via Gcc wrote:
>> >> As you can deduce from the (set_attr "cc" ..), only constraint
>> >> alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.
>> >
>> > Yes, I recognize that.
>> >
>> >> My first version of the port adds a post-reload splitter that adds a
>> >> (clobber (reg:CC REG_CC)) unconditionally, and it appears to work.
>> >
>> > Ouch, temporarily lying to gcc here.
>> >
>> >> If I
>> >> do want to add the clobber conditionally, would something like the below
>> >> be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
>> >> or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner 
>> >> way?
>> >
>> > I suggest having a look at what I did for the CRIS port.
>> > Check the git history.
>> >
>> > In short:
>> > - Add the clobber initially, to *all* patterns.
>> > - Add postreload splitters for the special combinations that
>> > don't clobber CC (ones without clobbering CC).
>> > - Use the old "cc" attribute to control and generate
>> > clobber/useful CC-setting alternatives (for various new CC_
>> > modes) with use of define_subst.
>>
>> This sounds like a great plan - the idea of always generating insn
>> variants for however many CCmodes are available, and then using
>> cc_enabled  to disable them selectively (based on the attribute value
>> corresponding to the alternative chosen) looks really neat. I did not
>> understand the purpose of subst_attr for "ccnz" "ccnzvc" and ""
>> though - wouldn't a (set_attr "cc_enabled", "...") do the same thing?
>
> You mean adding a whole new line with separate per-alternative
> settings instead of just a suffix to the name, per instruction,
> to get it compare-elim-optimized?  (Ok, plus a tweak to
> SELECT_CC_MODE; see a33649e).  That's not simpler to me, and
> error-prone without benefits.  Note that the "cc" attribute was
> already there from cc0 times, just as for AVR!
>
> But beware, the cc attributes *might* need tweaking, and also
> note that disabling an alternative for an insn may make gcc pick
> a subsequent one, which may be invalid when matching operands
> for the disabled alternative.
>
> But that's the simplistic and obvious reply, perhaps I'm
> misinterpreting and you mean something else?  For the purpose of
> the different CCmodes, see cris-modes.def, in particular the
> second paragraph, which I'm not repeating here.

My question was a lot more basic. From the internals documentation,
I understand that, with define_subst_attr for setcc, setnz and setnzc,
and the corresponding define_substs,

(define_insn "*movsi_internal ...)

results in a

(define_insn "*movsi_internal ...)

and in addition to that,

(define_insn "*movsi_internal_setcc ...) // output template of setcc_subst
(define_insn "*movsi_internal_setnz ...) // output template of setnz_subst
(define_insn "*movsi_internal_setnzvc ...) // output template of setnzvc_subst

I understood why this was done.

What I didn't understand was the (set-attr "cc")
part - as far I can tell, this results in (set_attr "cc_enabled" ...) in
all of the three substituted patterns, so I wondered why not just have
(set_attr "cc_enabled" ...) in the original define_insn instead.

I now realize that with (set-attr "cc"), the original
unsubstituted pattern will have only a (set_attr "cc" ...) and would
therefore not match the attr check for "enabled" - correctly so, as the
original insn pattern clobbers CRIS_CC0_REGNUM. Did I get that right?

>
>> Also, I already have a version that hides REG_CC until reload (based on
>> the suggestion in the wiki page), but I guess this approach will work
>> equally well with that too?
>
> Ow, I see it does!  I don't know, that's unchartered territory
> to me, but it does seem like visium was written that way.  It
> seems a bit wordy though; the define_split isn't necessary if
> you have the clobber from the start and use define_subst for the
> other alternatives.  You still need splitters for special-cases
> of insns where condition codes aren't affected though.  If these
> "special cases" approach being the norm, then I don't know.

Ok, I'll try the cris approach too, and see if it makes any difference.
>
> IMHO, introducing REG_CC clobbers only *after* reload seems like
> it could backfire.  Doing things behind gcc's back is what we're
> eliminating, not re-introducing, to be dogmatic.
>
> That wiki page mentions using define_subst to avoid all
> (near-)duplicates, but not in its examples.

Yes, I saw that too.

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj via Gcc]

Hans-Peter Nilsson writes:

> On Fri, 14 Aug 2020, Senthil Kumar Selvaraj via Gcc wrote:
>> As you can deduce from the (set_attr "cc" ..), only constraint
>> alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.
>
> Yes, I recognize that.
>
>> My first version of the port adds a post-reload splitter that adds a
>> (clobber (reg:CC REG_CC)) unconditionally, and it appears to work.
>
> Ouch, temporarily lying to gcc here.
>
>> If I
>> do want to add the clobber conditionally, would something like the below
>> be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
>> or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner way?
>
> I suggest having a look at what I did for the CRIS port.
> Check the git history.
>
> In short:
> - Add the clobber initially, to *all* patterns.
> - Add postreload splitters for the special combinations that
> don't clobber CC (ones without clobbering CC).
> - Use the old "cc" attribute to control and generate
> clobber/useful CC-setting alternatives (for various new CC_
> modes) with use of define_subst.

This sounds like a great plan - the idea of always generating insn
variants for however many CCmodes are available, and then using
cc_enabled  to disable them selectively (based on the attribute value
corresponding to the alternative chosen) looks really neat. I did not
understand the purpose of subst_attr for "ccnz" "ccnzvc" and ""
though - wouldn't a (set_attr "cc_enabled", "...") do the same thing?

Also, I already have a version that hides REG_CC until reload (based on
the suggestion in the wiki page), but I guess this approach will work
equally well with that too?

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj via Gcc]

Pip Cet writes:

> On Mon, Aug 17, 2020 at 7:31 AM Senthil Kumar Selvaraj
>  wrote:
>> > (define_split
>> >   [(parallel [(set (match_operand:ALL1 0 "nonimmediate_operand")
>> >  (match_operand:ALL1 1 "nox_general_operand"))
>> >   (clobber (reg:CC REG_CC))])]
>> >   "reload_completed && REG_P (operands[0]) && REG_P (operands[1])"
>> >   [(parallel [(set (match_dup 0)
>> >(match_dup 1))
>> >   (clobber (scratch:CC))])])
>> >
>> > and so on, for all four constraint alternatives that don't actually
>> > clobber REG_CC (and also for a fifth which only rarely clobbers
>> > REG_CC). That's duplicated code, of course.
>>
>> The (setattr "cc" ...) that is currently present for all
>> patterns accounts for the constraint alternatives,so  using
>> get_attr_cc to split to a (clobber) of either cc_reg_rtx or a
>> gen_rtx_SCRATCH (CCmode) appears to work.
>
> Thanks! Using an insn attribute is actually what I ended up doing
> (https://github.com/pipcet/gcc/commit/d4509afae9238e0ade4d3e1e97dd8577dae96115)
> :-)
>
> It's still confusing, IMHO, that insn attributes (but not the
> get_attr_alternative attribute which is mentioned in the
> documentation) are available when which_alternative is not.
>
>> (define_insn "*mov_insn"
>>   [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
>> ,q,r,*r")
>> (match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
>> Y00,Qm,r,q,i"))
>>(clobber (match_scratch:CC 2  "=X,X ,c   ,c 
>> ,X,X,c"))]
>
> Hmm. Technically, of course, clearing register 0 (a special case of
> the first alternative) would clobber the flags, but as it happens, the
> rewrite above produces the right clobber expression which is simply
> accepted by the "X"... I'm not sure whether anything else is trying to
> recognize such insns, but as it stands that define_insn would
> recognize the incorrect insn:
>
> [(set (reg:QI 0) (const_int 0))
>  (clobber (scratch:CC))]

get_cc_reg_clobber_rtx also looks at the insn itself (i.e. whatever
the erstwhile NOTICE_UPDATE_CC hook did), so if the cc attribute is LDI,
and operands[1] is const0_rtx, it would return cc_reg_rtx as the clobber reg.

AFAICT, some passes after reload verify if operands match constraints,
and that would work in this case - I'm not sure if the pattern you
mentioned could show up, outside of wrong splitters/peepholes in the md file.

Another approach would be to conservatively use a 'c' constraint and
clobber REG_CC for all reg-reg moves.

Regards
Senthil

Re: Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj via Gcc]

Pip Cet writes:

> On Sun, Aug 16, 2020 at 12:50 AM Segher Boessenkool
>  wrote:
>> On Sat, Aug 15, 2020 at 10:18:27AM +, Pip Cet wrote:
>> > > > What I'm currently doing is this:
>> > > >
>> > > > (define_split
>> > > >   [(set (match_operand 0 "nonimmediate_operand")
>> > > > (match_operand 1 "nox_general_operand"))]
>> > > >   "reload_completed && mov_clobbers_cc (insn, operands)"
>> > > >   [(parallel [(set (match_dup 0) (match_dup 1))
>> > > >   (clobber (reg:CC REG_CC))])])
>> > > >
>> > > > which, if I understand correctly, introduces the parallel-clobber
>> > > > expression only when necessary, at the same time as post-reload
>> > > > splitters introduce REG_CC references. Is that correct?
>> > >
>> > > Yes.  And this will work correctly if somehow you ensure that REG_CC
>> > > isn't live anywhere you introduce such a clobber.
>> >
>> > IIUC, the idea is that references to REG_CC, except for clobbers, are
>> > only introduced in the post-reload split pass, so it cannot be live
>> > before our define_split runs. Does that make sense?
>>
>> Yes, it does.  It has some huge restrictions (using the reg in inline
>> assembler can never work reliably, for example, so you'll have to
>> disallow that).
>
> Is there any approach that doesn't suffer from that problem? My
> understanding was that we need to allow reload to insert CC-clobbering
> insns on this (and many other) architectures, and that there are so
> many places where reload might choose to do so (including before and
> after inline asm) that using the register prior to reload just isn't
> possible. I'd be glad to be wrong, though :-)
>
> Is it true that reload chooses which constraint alternative is used
> for each insn? Is that information available somehow to post-reload
> splits? The problem is that the "X" constraint matches whatever's
> there already, and doesn't replace it with the (scratch:CC) expression
> that would work, so I can't rewrite those insns not to clobber the CC
> reg.
>
> For example, here's what I currently have:
>
> (define_expand "mov"
>   [(parallel [(set (match_operand:MOVMODE 0 "nonimmediate_operand" "")
>(match_operand:MOVMODE 1 "general_operand" ""))
>   (clobber (reg:CC REG_CC))])]
>   ...)
>
> (define_insn "mov_insn"
>[(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,r  ,d,Qm
>   ,r ,q,r,*r")
> (match_operand:ALL1 1 "nox_general_operand"   "r,Y00,n Ynn,r
> Y00,Qm,r,q,i"))
>(clobber (match_scratch:CC 2 "=X,c,X,c,c,X,X,c"))]
>   ...)
>
> That works, but it results in an incorrect CC clobber for, say,
> register-to-register movqi. For that, I'd need something like
>
> (define_split
>   [(parallel [(set (match_operand:ALL1 0 "nonimmediate_operand")
>  (match_operand:ALL1 1 "nox_general_operand"))
>   (clobber (reg:CC REG_CC))])]
>   "reload_completed && REG_P (operands[0]) && REG_P (operands[1])"
>   [(parallel [(set (match_dup 0)
>(match_dup 1))
>   (clobber (scratch:CC))])])
>
> and so on, for all four constraint alternatives that don't actually
> clobber REG_CC (and also for a fifth which only rarely clobbers
> REG_CC). That's duplicated code, of course.

The (setattr "cc" ...) that is currently present for all
patterns accounts for the constraint alternatives,so  using
get_attr_cc to split to a (clobber) of either cc_reg_rtx or a
gen_rtx_SCRATCH (CCmode) appears to work.

(define_insn_and_split "mov_insn"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
,q,r,*r")
(match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
Y00,Qm,r,q,i"))]
  "register_operand (operands[0], mode)
|| reg_or_0_operand (operands[1], mode)"
  "#"
  "&& reload_completed"
  [(parallel [(set (match_dup 0)
   (match_dup 1))
  (clobber (match_dup 2))])]
  {
operands[2] = get_cc_reg_clobber_rtx (curr_insn);
  }
  [(set_attr "cc" "ldi,none,clobber,clobber,none,none,clobber")])

(define_insn "*mov_insn"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
,q,r,*r")
(match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
Y00,Qm,r,q,i"))
   (clobber (match_scratch:CC 2  "=X,X ,c   ,c 
,X,X,c"))]
  "(register_operand (operands[0], mode)
|| reg_or_0_operand (operands[1], mode))
   && reload_completed"
  {
return output_movqi (insn, operands, NULL);
  }
  [(set_attr "length" "1,1,5,5,1,1,4")
   (set_attr "adjust_len" "mov8")])

As you mentioned elsewhere, keeping the clobber with "X" helps keep a
single pattern for both CC reg clobbering and non-clobbering insns.


>
> All this is at least somewhat academic: the code produced isn't
> drastically better after my cc conversion, but it's not usually any
> worse, and I'm still looking at assembler examples that are pessimized
> a little to find out how to fix them...
>
>> And earlier passes (like combine) cannot optimise this,
>
> I'm not sure what "this" 

Clobber REG_CC only for some constraint alternatives?

[Senthil Kumar Selvaraj via Gcc]

Hi,

  I'm working on porting AVR to MODE_CC, and there are quite a few
  patterns that clobber the condition code reg only for certain
  constraint alternatives. For e.g.,

(define_insn "mov_insn"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
,q,r,*r")
(match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
Y00,Qm,r,q,i"))]
 "register_operand (operands[0], mode)
 || reg_or_0_operand (operands[1], mode)"
 {
  return output_movqi (insn, operands, NULL);
 }
 [(set_attr "length" "1,1,5,5,1,1,4")
 (set_attr "adjust_len" "mov8")
 (set_attr "cc" "ldi,none,clobber,clobber,none,none,clobber")])

As you can deduce from the (set_attr "cc" ..), only constraint
alternatives 0,2,3 and 6 clobber CC - others leave it unchanged.

My first version of the port adds a post-reload splitter that adds a
(clobber (reg:CC REG_CC)) unconditionally, and it appears to work. If I
do want to add the clobber conditionally, would something like the below
be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx
or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner way?

(define_insn_and_split "mov_insn"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d,Qm   ,r 
,q,r,*r")
(match_operand:ALL1 1 "nox_general_operand"   "r Y00,n Ynn,r 
Y00,Qm,r,q,i"))]
  "register_operand (operands[0], mode)
|| reg_or_0_operand (operands[1], mode)"
  "#"
  "&& reload_completed"
  [(parallel [(set (match_dup 0)
   (match_dup 1))
  (clobber (match_dup 2))])]
  {
operands[2] = get_cc_reg_clobber_rtx (curr_insn);
  }
  [(set_attr "cc" "ldi,none,clobber,clobber,none,none,clobber")])

(define_insn "*mov_insn_clobber_flags"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,Qm   ,r ,*r")
(match_operand:ALL1 1 "nox_general_operand"   "r Y00,r Y00,Qm,i"))
   (clobber (reg:CC REG_CC))]
  "(register_operand (operands[0], mode)
|| reg_or_0_operand (operands[1], mode))
   && reload_completed"
  {
return output_movqi (insn, operands, NULL);
  }
  [(set_attr "length" "1,5,5,4")
   (set_attr "adjust_len" "mov8")])

(define_insn "*mov_insn_noclobber_flags"
  [(set (match_operand:ALL1 0 "nonimmediate_operand" "=r,d   ,q,r")
(match_operand:ALL1 1 "nox_general_operand"   "r,n Ynn,r,q"))
   (clobber (const_int 0))]
  "(register_operand (operands[0], mode)
|| reg_or_0_operand (operands[1], mode))
   && reload_completed"
  {
return output_movqi (insn, operands, NULL);
  }
  [(set_attr "length" "1,1,1,1")
   (set_attr "adjust_len" "mov8")])

Regards
Senthil

Re: [PATCH] emit-rtl.c: Allow splitting of RTX_FRAME_RELATED_P insns?

[Senthil Kumar Selvaraj via Gcc-patches]

Richard Sandiford writes:

> Senthil Kumar via Gcc-patches  writes:
>> Hi,
>>
>>   I'm working on converting the AVR backend to MODE_CC, following
>>   the steps described for case #2 in the CC0 transition wiki page,
>>   and I've implemented the first three bullet
>>   points (https://github.com/saaadhu/gcc-avr-cc0/tree/avr-cc0-squashed). With
>>   the below patch, there are zero regressions (for mega and xmega
>>   subarchs) compared to the current mainline, as of yesterday.
>>
>>   The wiki suggests using post-reload splitters, so that's the
>>   direction I took, but I ran into an issue where split_insn
>>   bails out early if RTX_FRAME_RELATED_P is true - this means
>>   that splits for REG_CC clobbering insns with
>>   RTX_FRAME_RELATED_P will never execute, resulting in a
>>   could-not-split insn ICE in the final stage.
>>
>>   I see that the recog.c:peep2_attempt allows splitting of a
>>   RTX_FRAME_RELATED_P insn, provided the result of the split is a
>>   single insn. Would it be ok to modify try_split also to
>>   allow those kinds of insns (tentative patch attached, code
>>   copied over from peep2_attempt, only setting old and new_insn)? Or is there
>>   a different approach to fix this?
>
> I agree there's no obvious reason why splitting to a single insn
> should be rejected but a peephole2 to a single instruction should be OK.
> And reusing the existing, tried-and-tested code is the way to go.
>
> But could you split the code out of peep2_attempt into a subroutine
> (probably still in recog.c) and reuse it in try_split?

How does the below patch look? Bootstrapped and reg tested on
x86_64-linux.
>
> BTW, just to check: is your email address in MAINTAINERS still correct?

It was out-of-date, yes - updated now.

Regards
Senthil


2020-08-13  Senthil Kumar Selvaraj  
   
gcc/ChangeLog:

* emit-rtl.c (try_split): Call copy_frame_info_to_split_insn
to split certain RTX_FRAME_RELATED_P insns.
* recog.c (copy_frame_info_to_split_insn): New function.
(peep2_attempt): Split copying of frame related info of
RTX_FRAME_RELATED_P insns into above function and call it.
* recog.h (copy_frame_info_to_split_insn): Declare it.

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index f9b0e9714d9..3706f0a03fd 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -3822,10 +3822,6 @@ try_split (rtx pat, rtx_insn *trial, int last)
   int njumps = 0;
   rtx_insn *call_insn = NULL;
 
-  /* We're not good at redistributing frame information.  */
-  if (RTX_FRAME_RELATED_P (trial))
-return trial;
-
   if (any_condjump_p (trial)
   && (note = find_reg_note (trial, REG_BR_PROB, 0)))
 split_branch_probability
@@ -3842,6 +3838,7 @@ try_split (rtx pat, rtx_insn *trial, int last)
   if (!seq)
 return trial;
 
+  int split_insn_count = 0;
   /* Avoid infinite loop if any insn of the result matches
  the original pattern.  */
   insn_last = seq;
@@ -3850,11 +3847,25 @@ try_split (rtx pat, rtx_insn *trial, int last)
   if (INSN_P (insn_last)
  && rtx_equal_p (PATTERN (insn_last), pat))
return trial;
+  split_insn_count++;
   if (!NEXT_INSN (insn_last))
break;
   insn_last = NEXT_INSN (insn_last);
 }
 
+  /* We're not good at redistributing frame information if
+ the split occurs before reload or if it results in more
+ than one insn.  */
+  if (RTX_FRAME_RELATED_P (trial))
+{
+  if (!reload_completed || split_insn_count != 1)
+return trial;
+
+  rtx_insn *new_insn = seq;
+  rtx_insn *old_insn = trial;
+  copy_frame_info_to_split_insn (old_insn, new_insn);
+}
+
   /* We will be adding the new sequence to the function.  The splitters
  may have introduced invalid RTL sharing, so unshare the sequence now.  */
   unshare_all_rtl_in_chain (seq);
diff --git a/gcc/recog.c b/gcc/recog.c
index 25f19b1b1cf..e024597f9d7 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -3277,6 +3277,78 @@ peep2_reinit_state (regset live)
   COPY_REG_SET (peep2_insn_data[MAX_INSNS_PER_PEEP2].live_before, live);
 }
 
+/* Copies frame related info of an insn (old_insn) to the single
+   insn (new_insn) that was obtained by splitting old_insn.  */
+
+void
+copy_frame_info_to_split_insn (rtx_insn *old_insn, rtx_insn *new_insn)
+{
+  bool any_note = false;
+  rtx note;
+
+  if (!RTX_FRAME_RELATED_P (old_insn))
+return;
+
+  RTX_FRAME_RELATED_P (new_insn) = 1;
+
+  /* Allow the backend to fill in a note during the split.  */
+  for (note = REG_NOTES (new_insn); note ; note = XEXP (note, 1))
+switch (REG_NOTE_KIND (note))
+  {
+  case REG_FRAME_RELATED_EXPR:
+  case REG_CFA_DEF_CFA:
+  case REG_CFA_ADJUST_CFA:
+  case REG_CFA_OFFSET:
+  case REG_CFA_REGISTER:
+  case REG_CFA_EXPRESSION:
+  case RE

[Committed] Update email address

[Senthil Kumar Selvaraj via Gcc-patches]

This patch updates my email address in the MAINTAINERS file

2020-08-12  Senthil Kumar Selvaraj  

	* MAINTAINERS: Update my email address.


diff --git MAINTAINERS MAINTAINERS
index 0b825c7ea6d..217ec9c9eca 100644
--- MAINTAINERS
+++ MAINTAINERS
@@ -588,7 +588,7 @@ Stefan Schulze Frielinghaus 

 Tilo Schwarz   
 Martin Sebor   
 Svein Seldal   
-Senthil Kumar Selvaraj 

+Senthil Kumar Selvaraj 
 Thiemo Seufer  
 Bill Seurer
 Tim Shen   

Re: [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array

[Senthil Kumar Selvaraj]

Jeff Law writes:

> On 08/06/2018 04:53 AM, Senthil Kumar Selvaraj wrote:
>> Ping!
>> 
>> Regards
>> Senthil
>> 
>> Senthil Kumar Selvaraj writes:
>> 
>>> Hi,
>>>
>>> The below patch fixes an ICE for the avr target when the setmemhi
>>> expander is involved.
>>>
>>> The setmemhi expander generated RTL ends up as an unrecognized insn
>>> if the alignment of the destination exceeds that of a QI
>>> mode const_int (127), AND the number of bytes to set fits in a QI
>>> mode const_int. The second condition prevents *clrmemhi from matching,
>>> and *clrmemqi does not match because it expects operand 3 (the alignment
>>> const_int rtx) to be QI mode, and a value of 128 or greater does not fit.
>>>   
>>> The patch fixes this by changing the *clrmemqi pattern to match a HI
>>> mode const_int, and also adds a testcase.
>>>
>>> Regression test showed no new failures, ok to commit to trunk?
>>>
>>> Regards
>>> Senthil
>>>
>>> gcc/ChangeLog:
>>> 
>>> 2018-07-18  Senthil Kumar Selvaraj  
>>> 
>>> PR target/85624
>>> * config/avr/avr.md (*clrmemqi): Change mode of operands[2]
>>> from QI to HI.
>>> 
>>> gcc/testsuite/ChangeLog:
>>> 
>>> 2018-07-18  Senthil Kumar Selvaraj  
>>> 
>>> PR target/85624
>>> * gcc.target/avr/pr85624.c: New test.
> Given there's also pattern clrmemhi it feels like you're papering over a
> bug elsewhere, possibly in the expanders.

clrmemhi and clrmemqi differ on the mode of the register used to hold
the number of bytes to clear. The setmemhi expander picks a QI or HI
mode reg depending on the width of the size operand, and the
clrmem{qi,hi} match on that.

The operand whose mode I modified represents the alignment of the
destination, and isn't actually used in the assembler template.

Regards
Senthil
>
> Ultimately I'll leave the decision here to the AVR maintainers through.
>
> jeff

Re: [Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array

[Senthil Kumar Selvaraj]

Ping!

Regards
Senthil

Senthil Kumar Selvaraj writes:

> Hi,
>
> The below patch fixes an ICE for the avr target when the setmemhi
> expander is involved.
>
> The setmemhi expander generated RTL ends up as an unrecognized insn
> if the alignment of the destination exceeds that of a QI
> mode const_int (127), AND the number of bytes to set fits in a QI
> mode const_int. The second condition prevents *clrmemhi from matching,
> and *clrmemqi does not match because it expects operand 3 (the alignment
> const_int rtx) to be QI mode, and a value of 128 or greater does not fit.
>   
> The patch fixes this by changing the *clrmemqi pattern to match a HI
> mode const_int, and also adds a testcase.
>
> Regression test showed no new failures, ok to commit to trunk?
>
> Regards
> Senthil
>
> gcc/ChangeLog:
> 
> 2018-07-18  Senthil Kumar Selvaraj  
> 
> PR target/85624
> * config/avr/avr.md (*clrmemqi): Change mode of operands[2]
>     from QI to HI.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-07-18  Senthil Kumar Selvaraj  
> 
> PR target/85624
> * gcc.target/avr/pr85624.c: New test.
>
> diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
> index e619e695418..644e3cfabc5 100644
> --- gcc/config/avr/avr.md
> +++ gcc/config/avr/avr.md
> @@ -1095,7 +1095,7 @@
>[(set (mem:BLK (match_operand:HI 0 "register_operand" "e"))
>  (const_int 0))
> (use (match_operand:QI 1 "register_operand" "r"))
> -   (use (match_operand:QI 2 "const_int_operand" "n"))
> +   (use (match_operand:HI 2 "const_int_operand" "n"))
> (clobber (match_scratch:HI 3 "=0"))
> (clobber (match_scratch:QI 4 "=&1"))]
>""
> diff --git gcc/testsuite/gcc.target/avr/pr85624.c 
> gcc/testsuite/gcc.target/avr/pr85624.c
> new file mode 100644
> index 000..ede2e80216a
> --- /dev/null
> +++ gcc/testsuite/gcc.target/avr/pr85624.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3" } */
> +
> +/* This testcase exposes PR85624. An alignment directive with
> +   a value greater than 127 on an array with dimensions that fit
> +   QImode causes an 'unrecognizable insn' ICE. Turns out clrmemqi
> +   did not match the pattern expanded by setmemhi, because it
> +   assumed the alignment val will fit in a QI. */
> +
> +int foo() {
> +  volatile int arr[3] __attribute__((aligned(128))) = {0};
> +  return arr[2];
> +}

Re: [Patch, avr, PR86635] Fix miscompilation with __memx and libgcc float function __gtsf2

[Senthil Kumar Selvaraj]

Senthil Kumar Selvaraj writes:

> The below patch fixes a miscompilation of function calls with__memx address 
> space
> arguments.
>
> For code like
>
> extern const  __memx float a;
> extern const float b;
>
> int diff () { return a > b; }
>
> when compiled with -Os, a is never loaded and passed in as an argument
> to the __gtsf2 libgcc function.
>
> Turns out that mov for the variable a expands into
>
> (insn 8 7 9 2 (parallel [
> (set (reg:SF 22 r22)
> (mem/u/c:SF (reg/f:PSI 47) [1 a+0 S4 A8 AS7]))
> (clobber (reg:SF 22 r22))
> (clobber (reg:QI 21 r21))
> (clobber (reg:HI 30 r30))
> ]) "test.c":4 36 {xloadsf_A}
>  (expr_list:REG_DEAD (reg/f:PSI 47)
> (expr_list:REG_UNUSED (reg:HI 30 r30)
> (expr_list:REG_EQUAL (mem/u/c:SF (symbol_ref:PSI ("a") [flags 
> 0xe40]  ) [1 a+0 S4 A8 AS7])
> (nil)
>
> The ud_dce pass sees this insn and deletes it as reg:SF r22 is both set
> and clobbered.
>
> Georg-Johann pointed out a similar issue (PR63633), and that was fixed
> by introducing a pseudo as the target of set. This patch does the same -
> adds an avr_emit2_fix_outputs for gen functions with 2 operands, that
> detects hard reg conflicts with clobbered regs and substitutes pseudos
> in their place.
>
> The patch also adds a testcase to verify a is actually read. Reg testing
> passed. Ok to commit to trunk?

Sent an out-of-date patch. Here's the right one.
>
> Regards
> Senthil
>
gcc/ChangeLog:

2018-07-25  Senthil Kumar Selvaraj  

* config/avr/avr-protos.h (avr_emit2_fix_outputs): New prototype.
  * config/avr/avr.c (avr_emit2_fix_outputs): New function.
  * config/avr/avr.md (mov): Wrap gen_xload_A call
with avr_emit2_fix_outputs.

gcc/testsuite/ChangeLog:

2018-07-25  Senthil Kumar Selvaraj  

* gcc.target/avr/torture/pr86635.c: New test.



diff --git gcc/config/avr/avr-protos.h gcc/config/avr/avr-protos.h
index 5622e9035a0..f8db418582e 100644
--- gcc/config/avr/avr-protos.h
+++ gcc/config/avr/avr-protos.h
@@ -135,6 +135,7 @@ regmask (machine_mode mode, unsigned regno)
 }
 
 extern void avr_fix_inputs (rtx*, unsigned, unsigned);
+extern bool avr_emit2_fix_outputs (rtx (*)(rtx,rtx), rtx*, unsigned, unsigned);
 extern bool avr_emit3_fix_outputs (rtx (*)(rtx,rtx,rtx), rtx*, unsigned, 
unsigned);
 
 extern rtx lpm_reg_rtx;
diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index 81c35e7fbc2..996d5187c52 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -13335,6 +13335,34 @@ avr_emit3_fix_outputs (rtx (*gen)(rtx,rtx,rtx), rtx 
*op,
   return avr_move_fixed_operands (op, hreg, opmask);
 }
 
+/* Same as avr_emit3_fix_outputs, but for 2 operands */
+bool
+avr_emit2_fix_outputs (rtx (*gen)(rtx,rtx), rtx *op,
+   unsigned opmask, unsigned rmask)
+{
+  const int n = 2;
+  rtx hreg[n];
+
+  /* It is letigimate for GEN to call this function, and in order not to
+ get self-recursive we use the following static kludge.  This is the
+ only way not to duplicate all expanders and to avoid ugly and
+ hard-to-maintain C-code instead of the much more appreciated RTL
+ representation as supplied by define_expand.  */
+  static bool lock = false;
+
+  gcc_assert (opmask < (1u << n));
+
+  if (lock)
+return false;
+
+  avr_fix_operands (op, hreg, opmask, rmask);
+
+  lock = true;
+  emit_insn (gen (op[0], op[1]));
+  lock = false;
+
+  return avr_move_fixed_operands (op, hreg, opmask);
+}
 
 /* Worker function for movmemhi expander.
XOP[0]  Destination as MEM:BLK
diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
index e619e695418..033a428e9f3 100644
--- gcc/config/avr/avr.md
+++ gcc/config/avr/avr.md
@@ -672,7 +672,14 @@
  ; insn-emit does not depend on the mode, it's all about operands. 
 */
   emit_insn (gen_xload8qi_A (dest, src));
 else
-  emit_insn (gen_xload_A (dest, src));
+  {
+operands[0] = dest; operands[1] = src;
+if (!avr_emit2_fix_outputs (gen_xload_A, operands, 1 << 0,
+  regmask (mode, 22)
+   | regmask (QImode, 21)
+   | regmask (HImode, REG_Z)))
+  FAIL;
+  }
 
 DONE;
   }
diff --git gcc/testsuite/gcc.target/avr/torture/pr86635.c 
gcc/testsuite/gcc.target/avr/torture/pr86635.c
new file mode 100644
index 000..f91367f7e7a
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/torture/pr86635.c
@@ -0,0 +1,9 @@
+/* { dg-do compile { target { ! avr_tiny } } } */
+/* { dg-options "-std=gnu99" } */
+
+extern const __memx float a;
+extern const float b;
+
+unsigned long diff () { return a > b; }
+
+/* { dg-final { scan-assembler "call __xload_4" } } */

[Patch, avr, PR86635] Fix miscompilation with __memx and libgcc float function __gtsf2

[Senthil Kumar Selvaraj]

The below patch fixes a miscompilation of function calls with__memx address 
space
arguments.

For code like

extern const  __memx float a;
extern const float b;

int diff () { return a > b; }

when compiled with -Os, a is never loaded and passed in as an argument
to the __gtsf2 libgcc function.

Turns out that mov for the variable a expands into

(insn 8 7 9 2 (parallel [
(set (reg:SF 22 r22)
(mem/u/c:SF (reg/f:PSI 47) [1 a+0 S4 A8 AS7]))
(clobber (reg:SF 22 r22))
(clobber (reg:QI 21 r21))
(clobber (reg:HI 30 r30))
]) "test.c":4 36 {xloadsf_A}
 (expr_list:REG_DEAD (reg/f:PSI 47)
(expr_list:REG_UNUSED (reg:HI 30 r30)
(expr_list:REG_EQUAL (mem/u/c:SF (symbol_ref:PSI ("a") [flags 
0xe40]  ) [1 a+0 S4 A8 AS7])
(nil)

The ud_dce pass sees this insn and deletes it as reg:SF r22 is both set
and clobbered.

Georg-Johann pointed out a similar issue (PR63633), and that was fixed
by introducing a pseudo as the target of set. This patch does the same -
adds an avr_emit2_fix_outputs for gen functions with 2 operands, that
detects hard reg conflicts with clobbered regs and substitutes pseudos
in their place.

The patch also adds a testcase to verify a is actually read. Reg testing
passed. Ok to commit to trunk?

Regards
Senthil

gcc/ChangeLog:

2018-07-25  Senthil Kumar Selvaraj  

* config/avr/avr-protos.h (avr_emit2_fix_outputs): New prototype.
* config/avr/avr.c (avr_emit2_fix_outputs): New function.
* config/avr/avr.md (mov): Wrap gen_xload_A call
  with avr_emit2_fix_outputs.

gcc/testsuite/ChangeLog:

2018-07-25  Senthil Kumar Selvaraj  

* gcc.target/avr/torture/pr86635.c: New test.


diff --git gcc/config/avr/avr-protos.h gcc/config/avr/avr-protos.h
index 5622e9035a0..f8db418582e 100644
--- gcc/config/avr/avr-protos.h
+++ gcc/config/avr/avr-protos.h
@@ -135,6 +135,7 @@ regmask (machine_mode mode, unsigned regno)
 }
 
 extern void avr_fix_inputs (rtx*, unsigned, unsigned);
+extern bool avr_emit2_fix_outputs (rtx (*)(rtx,rtx), rtx*, unsigned, unsigned);
 extern bool avr_emit3_fix_outputs (rtx (*)(rtx,rtx,rtx), rtx*, unsigned, 
unsigned);
 
 extern rtx lpm_reg_rtx;
diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index 81c35e7fbc2..996d5187c52 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -13335,6 +13335,34 @@ avr_emit3_fix_outputs (rtx (*gen)(rtx,rtx,rtx), rtx 
*op,
   return avr_move_fixed_operands (op, hreg, opmask);
 }
 
+/* Same as avr_emit3_fix_outputs, but for 2 operands */
+bool
+avr_emit2_fix_outputs (rtx (*gen)(rtx,rtx), rtx *op,
+   unsigned opmask, unsigned rmask)
+{
+  const int n = 2;
+  rtx hreg[n];
+
+  /* It is letigimate for GEN to call this function, and in order not to
+ get self-recursive we use the following static kludge.  This is the
+ only way not to duplicate all expanders and to avoid ugly and
+ hard-to-maintain C-code instead of the much more appreciated RTL
+ representation as supplied by define_expand.  */
+  static bool lock = false;
+
+  gcc_assert (opmask < (1u << n));
+
+  if (lock)
+return false;
+
+  avr_fix_operands (op, hreg, opmask, rmask);
+
+  lock = true;
+  emit_insn (gen (op[0], op[1]));
+  lock = false;
+
+  return avr_move_fixed_operands (op, hreg, opmask);
+}
 
 /* Worker function for movmemhi expander.
XOP[0]  Destination as MEM:BLK
diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
index e619e695418..5ff4e490a21 100644
--- gcc/config/avr/avr.md
+++ gcc/config/avr/avr.md
@@ -672,7 +672,11 @@
  ; insn-emit does not depend on the mode, it's all about operands. 
 */
   emit_insn (gen_xload8qi_A (dest, src));
 else
-  emit_insn (gen_xload_A (dest, src));
+  if (!avr_emit2_fix_outputs (gen_xload_A, operands, 1 << 0,
+  regmask (mode, 22)
+   | regmask (QImode, 21)
+   | regmask (HImode, REG_Z)))
+FAIL;
 
 DONE;
   }
diff --git gcc/testsuite/gcc.target/avr/torture/pr86635.c 
gcc/testsuite/gcc.target/avr/torture/pr86635.c
new file mode 100644
index 000..f91367f7e7a
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/torture/pr86635.c
@@ -0,0 +1,9 @@
+/* { dg-do compile { target { ! avr_tiny } } } */
+/* { dg-options "-std=gnu99" } */
+
+extern const __memx float a;
+extern const float b;
+
+unsigned long diff () { return a > b; }
+
+/* { dg-final { scan-assembler "call __xload_4" } } */

[Patch, avr, PR85624] - Fix ICE when initializing 128-byte aligned array

[Senthil Kumar Selvaraj]

Hi,

The below patch fixes an ICE for the avr target when the setmemhi
expander is involved.

The setmemhi expander generated RTL ends up as an unrecognized insn
if the alignment of the destination exceeds that of a QI
mode const_int (127), AND the number of bytes to set fits in a QI
mode const_int. The second condition prevents *clrmemhi from matching,
and *clrmemqi does not match because it expects operand 3 (the alignment
const_int rtx) to be QI mode, and a value of 128 or greater does not fit.
  
The patch fixes this by changing the *clrmemqi pattern to match a HI
mode const_int, and also adds a testcase.

Regression test showed no new failures, ok to commit to trunk?

Regards
Senthil

gcc/ChangeLog:

2018-07-18  Senthil Kumar Selvaraj  

PR target/85624
* config/avr/avr.md (*clrmemqi): Change mode of operands[2]
from QI to HI.

gcc/testsuite/ChangeLog:

2018-07-18  Senthil Kumar Selvaraj  

PR target/85624
* gcc.target/avr/pr85624.c: New test.

diff --git gcc/config/avr/avr.md gcc/config/avr/avr.md
index e619e695418..644e3cfabc5 100644
--- gcc/config/avr/avr.md
+++ gcc/config/avr/avr.md
@@ -1095,7 +1095,7 @@
   [(set (mem:BLK (match_operand:HI 0 "register_operand" "e"))
 (const_int 0))
(use (match_operand:QI 1 "register_operand" "r"))
-   (use (match_operand:QI 2 "const_int_operand" "n"))
+   (use (match_operand:HI 2 "const_int_operand" "n"))
(clobber (match_scratch:HI 3 "=0"))
(clobber (match_scratch:QI 4 "=&1"))]
   ""
diff --git gcc/testsuite/gcc.target/avr/pr85624.c 
gcc/testsuite/gcc.target/avr/pr85624.c
new file mode 100644
index 000..ede2e80216a
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr85624.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+/* This testcase exposes PR85624. An alignment directive with
+   a value greater than 127 on an array with dimensions that fit
+   QImode causes an 'unrecognizable insn' ICE. Turns out clrmemqi
+   did not match the pattern expanded by setmemhi, because it
+   assumed the alignment val will fit in a QI. */
+
+int foo() {
+  volatile int arr[3] __attribute__((aligned(128))) = {0};
+  return arr[2];
+}

[Patch, testsuite, committed] Fix bogus builtin-snprintf-warn-3.c failure for avr

[Senthil Kumar Selvaraj]

Hi,

  The below patch fixes failures in builtin-snprintf-warn-3.c for the
  avr target.

  The test declares a struct with an array member that has INT_MAX/32767
  elements. This causes a "type xxx is too large" error for targets like
  the avr, which have pointers smaller or equal to (16 bit) int size.

  Fixed by marking the test as unsupported for targets with ptr size <
  32. Committed as obvious.

Regards
Senthil

gcc/testsuite/

2017-05-23  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/tree-ssa/builtin-snprintf-warn-3.c: Require ptr32plus.

Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c
===
--- gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c (revision 
248360)
+++ gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c (working copy)
@@ -1,6 +1,7 @@
 /* PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning
{ dg-do compile }
-   { dg-options "-O2 -Wformat -Wformat-truncation=2 -ftrack-macro-expansion=0" 
} */
+   { dg-options "-O2 -Wformat -Wformat-truncation=2 -ftrack-macro-expansion=0" 
} 
+   { dg-require-effective-target ptr32plus } */
 
 typedef __SIZE_TYPE__  size_t;
 typedef __WCHAR_TYPE__ wchar_t;

[Patch, testsuite, committed] Fix bogus pr78886.c for avr

[Senthil Kumar Selvaraj]

Hi,

  The test declares malloc with an unsigned long parameter. This causes
  a warning for avr, as it's size_t is only unsigned int.

  Fixed by typdef'ing __SIZE_TYPE__ to size_t and using it in the malloc
  function's declaration.

  Committed as obvious.

Regards
Senthil

gcc/testsuite/ChangeLog

2017-05-17  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/tree-ssa/pr78886.c: Use __SIZE_TYPE__ instead of
unsigned long.

Index: gcc/testsuite/gcc.dg/tree-ssa/pr78886.c
===
--- gcc/testsuite/gcc.dg/tree-ssa/pr78886.c (revision 248137)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr78886.c (working copy)
@@ -1,7 +1,9 @@
 /* { dg-do compile } */
 /* { dg-options "-O2" } */
-void *malloc(unsigned long x);
 
+__extension__ typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t x);
+
 void foo(void)
 {
  volatile int i;

[Patch, testsuite, committed] Fix cunroll-13.c failure for avr

[Senthil Kumar Selvaraj]

Hi,

  The test reports bogus failures because the loop variable i is declared
  as int, and the constant expected in the dump doesn't fit in an int for avr.

  Fixed by explicitly using __INT32_TYPE__ for targets with __SIZEOF_INT__ < 4.

  Committed to trunk as obvious.

Regards
Senthil

gcc/testsuite/
2017-05-09  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/tree-ssa/cunroll-13.c: Use __INT32_TYPE__ for
for targets with __SIZEOF_INT__ < 4.


diff --git gcc/testsuite/gcc.dg/tree-ssa/cunroll-13.c 
gcc/testsuite/gcc.dg/tree-ssa/cunroll-13.c
index f3fe8b51468..904e6dc075b 100644
--- gcc/testsuite/gcc.dg/tree-ssa/cunroll-13.c
+++ gcc/testsuite/gcc.dg/tree-ssa/cunroll-13.c
@@ -1,10 +1,17 @@
 /* { dg-do compile } */
 /* { dg-options "-O3 -fdisable-tree-evrp -fdisable-tree-cunrolli 
-fdisable-tree-vrp1 -fdump-tree-cunroll-blocks-details" } */
+
+#if __SIZEOF_INT__ < 4
+__extension__ typedef __INT32_TYPE__ i32;
+#else
+typedef int i32;
+#endif
+
 struct a {int a[8];int b;};
 void
 t(struct a *a)
 {
-  for (int i=0;i<123456 && a->a[i];i++)
+  for (i32 i=0;i<123456 && a->a[i];i++)
 a->a[i]++;
 }
 /* This pass relies on the fact that we do not eliminate the redundant test 
for i early.

[Patch, testsuite] Fix bogus pr78138.c failure for avr

[Senthil Kumar Selvaraj]

Hi,

  The trivial patch below fixes a bogus testsuite failure
  (gcc.dg/pr78138.c) for the avr target.

  The declaration for memcpy had the size parameter declared as 
  unsigned long. For avr, __SIZE_TYPE__ is unsigned int, and 
  this caused a builtin-declaration-mismatch warning, resulting
  in a couple of FAILs.

  The patch fixes that by typedef'ing __SIZE_TYPE__ to size_t and
  using size_t as the type for memcpy's third parameter.

  Committed to trunk as obvious.

Regards
Senthil

gcc/testsuite/ChangeLog

2017-05-02  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/pr78138.c: Use __SIZE_TYPE__ instead of
unsigned long.

Index: gcc.dg/pr78138.c
===
--- gcc.dg/pr78138.c(revision 247481)
+++ gcc.dg/pr78138.c(working copy)
@@ -5,7 +5,9 @@
 
 char d [5];
 
-void* memcpy (void*, const void*, unsigned long);
+__extension__ typedef __SIZE_TYPE__ size_t;
+
+void* memcpy (void*, const void*, size_t);
 extern char* strcpy (char*, const char*);
 
 void f (int i, int j)

[Patch, testsuite] Fix failing attr-alloc_size-10.c for avr

[Senthil Kumar Selvaraj]

Hi,

Integer promotion combined with equal sizes for short and int (16 bits)
causes overflow warnings when expanding the TEST macro for USHRT_MAX.

Fixed by explicitly disabling overflow warnings for targets with !int32plus.

Committed as obvious.

Regards
Senthil


gcc/testsuite/ChangeLog

2017-04-25  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/attr-alloc_size-10.c: Ignore overflow warnings
for targets with int size less than 32 bits.


Index: gcc/testsuite/gcc.dg/attr-alloc_size-10.c
===
--- gcc/testsuite/gcc.dg/attr-alloc_size-10.c   (revision 247191)
+++ gcc/testsuite/gcc.dg/attr-alloc_size-10.c   (working copy)
@@ -4,7 +4,8 @@
range.
 
{ dg-do compile }
-   { dg-options "-O2 -Walloc-size-larger-than=12" } */
+   { dg-options "-O2 -Walloc-size-larger-than=12" } 
+   { dg-options "-Wno-overflow" { target { ! int32plus } } } */
 
 #define SCHAR_MAX __SCHAR_MAX__
 #define SCHAR_MIN (-SCHAR_MAX - 1)

[Patch, testsuite] Skip pr80170.c for non-ptr32plus targets

[Senthil Kumar Selvaraj]

Hi,

  The below patch skips gcc.dg/pr80170.c for targets with pointer size
  less than 32. The testcase uses pointer offsets that are 32 bit or
  higher, and this ends up triggering an ICE because a ptrofftype_p
  assert fires for non-ptr32 plus targets.

  Committed as obvious.

Regards
Senthil

gcc/testsuite/ChangeLog

2017-04-19  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/pr80170.c: Require ptr32plus.


Index: gcc.dg/pr80170.c
===
--- gcc.dg/pr80170.c(revision 247010)
+++ gcc.dg/pr80170.c(working copy)
@@ -1,5 +1,6 @@
 /* { dg-do run } */
 /* { dg-options "-fgimple -O2 -ftree-slp-vectorize" } */
+/* { dg-require-effective-target ptr32plus } *
 
 struct  A
 {

[Patch, testsuite] Fix broken pr80341.c for avr

[Senthil Kumar Selvaraj]

Hi,

  This patch skips pr80341.c for targets with int size less than 32 bits.
  The assertion in the testcase holds only if sizeof(int) > sizeof(short), 
  which isn't true for smaller int size targets like the avr.

  Specifically, after integer promotion, the "usual arithmetic
  conversion" of the unsigned short to signed int doesn't occur, and
  this causes the test to fail.

  Committed as obvious.

Regards
Senthil

2017-04-19  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/torture/pr80341.c: Require int32plus.

Index: gcc.dg/torture/pr80341.c
===
--- gcc.dg/torture/pr80341.c(revision 246991)
+++ gcc.dg/torture/pr80341.c(working copy)
@@ -1,5 +1,6 @@
 /* { dg-do run } */
 /* { dg-additional-options "-Wno-overflow" } */
+/* { dg-require-effective-target int32plus } */
 
 const signed char c = -84;
 signed char s;

[Patch, testsuite] Fix failing builtin-sprintf-warn-{3,10}.c for avr

[Senthil Kumar Selvaraj]

Hi,

  This patch fixes a whole bunch of failures reported for
  gcc.dg/tree-ssa/builtin-sprintf-warn-{3,10}.c for the avr target.

  builtin-sprintf-warn-10.c fails because the bounds in the warning
  messages expect 4 digit wide exponents i.e. __DBL_MAX_EXP__ > 999.
  For the avr, floats and doubles are both 32 bits wide, __DBL_MAX_EXP__
  == 128, and the max number of exponent digits can only be 3 .
  The computed size thus ends up one short of the value the test
  expects. The patch makes the test run only for targets with double64plus.

  builtin-sprintf-warn-3.c fails because the test appears to assume all
  non lp64 targets to be ilp32. For the avr, pointer size and int size
  are equal, but both are 16 bits, not 32. The patch fixes this by
  explicitly adding avr to the dejagnu selector.

  Ok for trunk?

Regards
Senthil

gcc/testsuite/ChangeLog:

2017-04-06  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/tree-ssa/builtin-sprintf-warn-10.c: Require double64plus.
* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (void test_too_large): Add
  avr-*-* to non-lp64 selector.


diff --git gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-10.c 
gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-10.c
index 1213e89f7bb..30599ad04dc 100644
--- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-10.c
+++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-10.c
@@ -2,6 +2,7 @@
Test to verify the correctness of ranges of output computed for floating
point directives.
{ dg-do compile }
+   { dg-require-effective-target double64plus }
{ dg-options "-O2 -Wformat -Wformat-overflow -ftrack-macro-expansion=0" } */
 
 typedef __builtin_va_list va_list;
diff --git gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c 
gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
index 72ec3afaa41..9db7ad74f37 100644
--- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
+++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
@@ -358,19 +358,19 @@ void test_too_large (char *d, int x, __builtin_va_list va)
 
   __builtin_snprintf (d, imax,"%c", x);
   __builtin_snprintf (d, imax_p1, "%c", x);   /* { dg-warning "specified bound 
\[0-9\]+ exceeds .INT_MAX." "INT_MAX + 1" { target lp64 } } */
-  /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size" 
"INT_MAX + 1" { target { ilp32 } } .-1 } */
+  /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size" 
"INT_MAX + 1" { target { { avr-*-* } || ilp32 } } .-1 } */
 
   __builtin_vsnprintf (d, imax,"%c", va);
   __builtin_vsnprintf (d, imax_p1, "%c", va);   /* { dg-warning "specified 
bound \[0-9\]+ exceeds .INT_MAX." "INT_MAX + 1" { target lp64 } } */
-  /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size" 
"INT_MAX + 1" { target { ilp32 } } .-1 } */
+  /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size" 
"INT_MAX + 1" { target { { avr-*-* } || ilp32 } } .-1 } */
 
   __builtin___snprintf_chk (d, imax,0, imax,"%c", x);
   __builtin___snprintf_chk (d, imax_p1, 0, imax_p1, "%c", x);   /* { 
dg-warning "specified bound \[0-9\]+ exceeds .INT_MAX." "INT_MAX + 1" { target 
lp64 } } */
-  /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size" 
"INT_MAX + 1" { target { ilp32 } } .-1 } */
+  /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size" 
"INT_MAX + 1" { target { { avr-*-* } || ilp32 } } .-1 } */
 
   __builtin___vsnprintf_chk (d, imax,0, imax,"%c", va);
   __builtin___vsnprintf_chk (d, imax_p1, 0, imax_p1, "%c", va);   /* { 
dg-warning "specified bound \[0-9\]+ exceeds .INT_MAX." "INT_MAX + 1" { target 
lp64 } } */
-  /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size" 
"INT_MAX + 1" { target { ilp32 } } .-1 } */
+  /* { dg-warning "specified bound \[0-9\]+ exceeds maximum object size" 
"INT_MAX + 1" { target { { avr-*-* } || ilp32 } } .-1 } */
 
   const size_t ptrmax = __PTRDIFF_MAX__;
   const size_t ptrmax_m1 = ptrmax - 1;

[Patch, testsuite] Fix more failing tests for avr

[Senthil Kumar Selvaraj]

Hi,

  The below trivial patch fixes some more testsuite failures for the avr
  target. They fail for avr because they assume ints are 32 bits or
  wider. The patch uses explicit __{U}INT32_TYPE__ for targets with
  smaller int size.

  Committed as obvious.

Regards
Senthil

2017-03-28  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.c-torture/execute/pr79121.c:Use __{U}INT32_TYPE__ for targets
with sizeof(int) < 4.
* gcc.c-torture/execute/pr79737-1.c (struct S): Likewise.
* gcc.c-torture/execute/pr79737-2.c: Likewise.
* gcc.dg/torture/pr79777.c: Likewise.
* gcc.dg/torture/pr79910.c: Likewise.

Index: gcc.c-torture/execute/pr79121.c
===
--- gcc.c-torture/execute/pr79121.c (revision 246528)
+++ gcc.c-torture/execute/pr79121.c (working copy)
@@ -1,21 +1,29 @@
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __UINT32_TYPE__ uint32_t;
+  __extension__ typedef __INT32_TYPE__ int32_t;
+#else
+  typedef unsigned uint32_t;
+  typedef int int32_t;
+#endif
+
 extern void abort (void);
 
-__attribute__ ((noinline, noclone)) unsigned long long f1 (int x)
+__attribute__ ((noinline, noclone)) unsigned long long f1 (int32_t x)
 {
   return ((unsigned long long) x) << 4;
 }
 
-__attribute__ ((noinline, noclone)) long long f2 (unsigned x)
+__attribute__ ((noinline, noclone)) long long f2 (uint32_t x)
 {
   return ((long long) x) << 4;
 }
 
-__attribute__ ((noinline, noclone)) unsigned long long f3 (unsigned x)
+__attribute__ ((noinline, noclone)) unsigned long long f3 (uint32_t x)
 {
   return ((unsigned long long) x) << 4;
 }
 
-__attribute__ ((noinline, noclone)) long long f4 (int x)
+__attribute__ ((noinline, noclone)) long long f4 (int32_t x)
 {
   return ((long long) x) << 4;
 }
Index: gcc.c-torture/execute/pr79737-1.c
===
--- gcc.c-torture/execute/pr79737-1.c   (revision 246528)
+++ gcc.c-torture/execute/pr79737-1.c   (working copy)
@@ -1,13 +1,19 @@
 /* PR tree-optimization/79737 */
 
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __INT32_TYPE__ int32_t;
+#else
+  typedef int int32_t;
+#endif
+
 #pragma pack(1)
 struct S
 {
-  int b:18;
-  int c:1;
-  int d:24;
-  int e:15;
-  int f:14;
+  int32_t b:18;
+  int32_t c:1;
+  int32_t d:24;
+  int32_t e:15;
+  int32_t f:14;
 } i;
 int g, j, k;
 static struct S h;
Index: gcc.c-torture/execute/pr79737-2.c
===
--- gcc.c-torture/execute/pr79737-2.c   (revision 246528)
+++ gcc.c-torture/execute/pr79737-2.c   (working copy)
@@ -1,13 +1,19 @@
 /* PR tree-optimization/79737 */
 
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __INT32_TYPE__ int32_t;
+#else
+  typedef int int32_t;
+#endif
+
 #pragma pack(1)
 struct S
 {
-  int b:18;
-  int c:1;
-  int d:24;
-  int e:15;
-  int f:14;
+  int32_t b:18;
+  int32_t c:1;
+  int32_t d:24;
+  int32_t e:15;
+  int32_t f:14;
 } i, j;
 
 void
Index: gcc.dg/torture/pr79777.c
===
--- gcc.dg/torture/pr79777.c(revision 246528)
+++ gcc.dg/torture/pr79777.c(working copy)
@@ -1,9 +1,14 @@
 /* { dg-do compile } */
 
 typedef unsigned short __u16;
-typedef unsigned int __u32;
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __UINT32_TYPE__ __u32;
+  __extension__ typedef __UINT32_TYPE__ u32;
+#else
+  typedef unsigned int __u32;
+  typedef unsigned int u32;
+#endif
 typedef unsigned char u8;
-typedef unsigned int u32;
 typedef __u16 __le16;
 typedef __u32 __le32;
 typedef u32 secno;
Index: gcc.dg/torture/pr79910.c
===
--- gcc.dg/torture/pr79910.c(revision 246528)
+++ gcc.dg/torture/pr79910.c(working copy)
@@ -2,7 +2,11 @@
 /* { dg-additional-options "-fweb" } */
 
 typedef unsigned char u8;
-typedef unsigned int u32;
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __UINT32_TYPE__ u32;
+#else
+  typedef unsigned int u32;
+#endif
 typedef unsigned long long u64;
 int a;
 

[Patch, testsuite] Fix failing overflow-1.c for avr

[Senthil Kumar Selvaraj]

Hi,

The test assumes 32 bit ints, and expects a constant in the
dump that is only valid for 32 bit ints. This trivial patch
fixes that by explicitly specifying __UINT32_TYPE__ as the type.

Committed as obvious.

Regards
Senthil

gcc/testsuite/ChangeLog

2017-03-21  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/tree-ssa/overflow-1.c: Use __UINT32_TYPE__ for targets
with sizeof(int) < 4.

diff --git gcc/testsuite/gcc.dg/tree-ssa/overflow-1.c 
gcc/testsuite/gcc.dg/tree-ssa/overflow-1.c
index e126609c53d9..b664d0f120aa 100644
--- gcc/testsuite/gcc.dg/tree-ssa/overflow-1.c
+++ gcc/testsuite/gcc.dg/tree-ssa/overflow-1.c
@@ -1,14 +1,20 @@
 /* { dg-do compile } */
 /* { dg-options "-O -fdump-tree-optimized" } */
 
-int f(unsigned a){
-unsigned b=5;
-unsigned c=a-b;
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __UINT32_TYPE__ uint32_t;
+#else
+  typedef unsigned uint32_t;
+#endif
+
+int f(uint32_t a){
+uint32_t b=5;
+uint32_t c=a-b;
 return c>a;
 }
-int g(unsigned a){
-unsigned b=32;
-unsigned c=a+b;
+int g(uint32_t a){
+uint32_t b=32;
+uint32_t c=a+b;
 return c<a;
 }
 

Missed optimizations at -Os

[Senthil Kumar Selvaraj]

Hi,

  For this (reduced) test case

  
extern int x, y, z;
void foo(void);
void bar(void);
void blah(void);

void test (void)
{
  int flag = 0;
  flag = ((x && y) || z);

  if (flag && x && y) {
 bar();
  }
}

  I expected gcc -Os (x86_64, if it matters) to generate code equivalent to

if (x && y)
  bar();


  Instead, I get

test():
mov eax, DWORD PTR x[rip]
testeax, eax
je  .L2
cmp DWORD PTR y[rip], 0
jne .L3
.L2:
cmp DWORD PTR z[rip], 0
je  .L1
testeax, eax
je  .L1
.L3:
cmp DWORD PTR y[rip], 0
je  .L1
jmp bar()
.L1:
ret

At -O1 and above though, I get what I expected. At -O3
test():
mov edx, DWORD PTR x[rip]
testedx, edx
je  .L1
mov eax, DWORD PTR y[rip]
testeax, eax
je  .L1
jmp bar()
.L1:
rep ret


Tracing through the dumps, I see that dom2 is where the gimple starts
diverging. At -O3, dom2 clones the bb that tests z into two copies, and
I guess that enables jump threading and subsequent dse to optimize away the
second (redundant) check for x and y, as also the check for z. At -Os,
dom2 doesn't attemp the bb clone as it thinks it would increase code
size. 

I have two questions.

1. Is the analysis right? Is there anything that can be done to fix this?

2. If nothing can be done to fix this, is there some pass that can
rewire goto  in  to goto ?

Regards
Senthil


.dom2 at Os


test ()
{
  int x.1_1;
  int y.2_2;
  int z.3_3;
  int y.5_4;
  _Bool _8;
  _Bool _9;
  _Bool _10;

   [100.00%]:
  x.1_1 = x;
  if (x.1_1 != 0)
goto ; [50.00%]
  else
goto ; [50.00%]

   [50.00%]:
  y.2_2 = y;
  if (y.2_2 != 0)
goto ; [50.00%]
  else
goto ; [50.00%]

   [75.00%]:
  z.3_3 = z;
  _8 = x.1_1 != 0;
  _9 = z.3_3 != 0;
  _10 = _8 & _9;
  if (_10 != 0)
goto ; [25.60%]
  else
goto ; [74.40%]

   [35.37%]:
  y.5_4 = y;
  if (y.5_4 != 0)
goto ; [48.99%]
  else
goto ; [51.01%]

   [17.33%]:
  bar ();

   [100.00%]:
  return;

}

.dom2 at O3


test ()
{
  int x.1_1;
  int y.2_2;
  int z.3_11;
  _Bool _12;
  _Bool _13;
  _Bool _14;
  int y.5_15;
  int z.3_16;
  _Bool _17;
  _Bool _18;
  _Bool _19;
  int y.5_20;

   [100.00%]:
  x.1_1 = x;
  if (x.1_1 != 0)
goto ; [50.00%]
  else
goto ; [50.00%]

   [50.00%]:
  y.2_2 = y;
  if (y.2_2 != 0)
goto ; [50.00%]
  else
goto ; [50.00%]

   [100.00%]:
  return;

   [50.00%]:
  z.3_11 = z;
  _12 = x.1_1 != 0;
  _13 = z.3_11 != 0;
  _14 = _12 & _13;
  goto ; [100.00%]

   [18.04%]:
  y.5_15 = y;
  goto ; [100.00%]

   [25.00%]:
  z.3_16 = z;
  _17 = x.1_1 != 0;
  _18 = z.3_16 != 0;
  _19 = _17 & _18;
  if (_19 != 0)
goto ; [72.17%]
  else
goto ; [27.83%]

   [25.00%]:
  y.5_20 = y;
  bar ();
  goto ; [100.00%]

}

Re: Worse code after bbro?

[Senthil Kumar Selvaraj]

Segher Boessenkool writes:

> On Wed, Jan 04, 2017 at 10:05:49AM +0100, Richard Biener wrote:
>> > The code size is identical, but the trunk version executes one more
>> > instruction everytime the loop runs (explicit jump to .L5 with trunk vs
>> > fallthrough with 4.8) - it's faster only if the loop never runs. This
>> > happens irrespective of the memory clobber inline assembler statement.
>
> With -Os you've asked for smaller code, not faster code.
>
> All of the block reordering is based on heuristics -- there is no polynomial
> time and space algorithm to do it optimally, let alone the linear time and
> space we need in GCC -- so there always will be cases we do not handle
> optimally.  -Os does not get as much attention as -O2 etc., as well.
>
> OTOH this seems to be a pretty common case that we could handle.  Please
> open a PR to keep track of this?
>

Filed PR 79012 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79012)

Regards
Senthil

Worse code after bbro?

[Senthil Kumar Selvaraj]

Hi,

  For this C code (slightly modified from PR 30908)

void wait(int i)
{
while (i-- > 0)
asm volatile("nop" ::: "memory");
}

  gcc 4.8 at -Os produces

jmp .L2
.L3:
nop
decl%edi
.L2:
testl   %edi, %edi
jg  .L3
ret

whereas gcc trunk (and 4.9 onwards, from a quick check) produces

.L2:
testl   %edi, %edi
jle .L5
nop
decl%edi
jmp .L2
.L5:
ret

The code size is identical, but the trunk version executes one more
instruction everytime the loop runs (explicit jump to .L5 with trunk vs
fallthrough with 4.8) - it's faster only if the loop never runs. This
happens irrespective of the memory clobber inline assembler statement.

Digging into the dump files, I found that the transformation occurs in
the bb reorder pass, when it calls cfg_layout_initialize, which
eventually calls try_redirect_by_replacing_jump with in_cfglayout set to
true. That function then removes the jump and causes the RTL
transformation that eventually results in slower code.

Is this intentional? If not, what would be the best way to fix this?

Regards
Senthil

RTL before and after bbro.

Before:

(jump_insn 24 6 25 2 (set (pc)
(label_ref 15)) "pr30908.c":3 678 {jump}
 (nil)
 -> 15)
(barrier 25 24 17)
(code_label 17 25 12 3 3 "" [1 uses])
(note 12 17 13 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
(insn 13 12 14 3 (parallel [
(asm_operands/v ("nop") ("") 0 []
 []
 [] pr30908.c:4)
(clobber (mem:BLK (scratch) [0  A8]))
(clobber (reg:CCFP 18 fpsr))
(clobber (reg:CC 17 flags))
]) "pr30908.c":4 -1
 (expr_list:REG_UNUSED (reg:CCFP 18 fpsr)
(expr_list:REG_UNUSED (reg:CC 17 flags)
(nil
(insn 14 13 15 3 (parallel [
(set (reg:SI 5 di [orig:90 ivtmp.9 ] [90])
(plus:SI (reg:SI 5 di [orig:90 ivtmp.9 ] [90])
(const_int -1 [0x])))
(clobber (reg:CC 17 flags))
]) 210 {*addsi_1}
 (expr_list:REG_UNUSED (reg:CC 17 flags)
(nil)))
(code_label 15 14 16 4 2 "" [1 uses])
(note 16 15 18 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(insn 18 16 19 4 (set (reg:CCNO 17 flags)
(compare:CCNO (reg:SI 5 di [orig:90 ivtmp.9 ] [90])
(const_int 0 [0]))) "pr30908.c":3 3 {*cmpsi_ccno_1}
 (nil))
(jump_insn 19 18 30 4 (set (pc)
(if_then_else (gt (reg:CCNO 17 flags)
(const_int 0 [0]))
(label_ref 17)
(pc))) "pr30908.c":3 646 {*jcc_1}
 (expr_list:REG_DEAD (reg:CCNO 17 flags)
(int_list:REG_BR_PROB 8500 (nil)))
 -> 17)
(note 30 19 28 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
(note 28 30 29 5 NOTE_INSN_EPILOGUE_BEG)
(jump_insn 29 28 31 5 (simple_return) "pr30908.c":5 708 {simple_return_internal}
 (nil)
 -> simple_return)

After:


(code_label 15 6 16 3 2 "" [1 uses])
(note 16 15 18 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
(insn 18 16 19 3 (set (reg:CCNO 17 flags)
(compare:CCNO (reg:SI 5 di [orig:90 ivtmp.9 ] [90])
(const_int 0 [0]))) "pr30908.c":3 3 {*cmpsi_ccno_1}
 (nil))
(jump_insn 19 18 12 3 (set (pc)
(if_then_else (le (reg:CCNO 17 flags)
(const_int 0 [0]))
(label_ref:DI 34)
(pc))) "pr30908.c":3 646 {*jcc_1}
 (expr_list:REG_DEAD (reg:CCNO 17 flags)
(int_list:REG_BR_PROB 1500 (nil)))
 -> 34)
(note 12 19 13 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(insn 13 12 14 4 (parallel [
(asm_operands/v ("nop") ("") 0 []
 []
 [] pr30908.c:4)
(clobber (mem:BLK (scratch) [0  A8]))
(clobber (reg:CCFP 18 fpsr))
(clobber (reg:CC 17 flags))
]) "pr30908.c":4 -1
 (expr_list:REG_UNUSED (reg:CCFP 18 fpsr)
(expr_list:REG_UNUSED (reg:CC 17 flags)
(nil
(insn 14 13 35 4 (parallel [
(set (reg:SI 5 di [orig:90 ivtmp.9 ] [90])
(plus:SI (reg:SI 5 di [orig:90 ivtmp.9 ] [90])
(const_int -1 [0x])))
(clobber (reg:CC 17 flags))
]) 210 {*addsi_1}
 (expr_list:REG_UNUSED (reg:CC 17 flags)
(nil)))
(jump_insn 35 14 36 4 (set (pc)
(label_ref 15)) -1
 (nil)
 -> 15)
(barrier 36 35 34)
(code_label 34 36 30 5 5 "" [1 uses])
(note 30 34 28 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
(note 28 30 29 5 NOTE_INSN_EPILOGUE_BEG)
(jump_insn 29 28 31 5 (simple_return) "pr30908.c":5 708 {simple_return_internal}
 (nil)
 -> simple_return)

[Patch, testsuite] Fix bogus pr31096-1.c failure for avr

[Senthil Kumar Selvaraj]

Hi,

  This patch fixes a bogus testsuite failure (gcc.dg/pr31096-1.c) for
  the avr target.

  The dump expects constants which would only be present if the target's
  int size is 32 bits.

  Fixed by explicitly using 32 bit ints for targets with __SIZEOF_INT__
  < 4. Committed to trunk as obvious.

Regards
Senthil


gcc/testsuite/ChangeLog

2016-11-29  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* testsuite/gcc.dg/pr31096-1.c: Use __{U,}INT32_TYPE__ for
targets with sizeof(int) < 4.


Index: gcc/testsuite/gcc.dg/pr31096-1.c
===
--- gcc/testsuite/gcc.dg/pr31096-1.c(revision 242953)
+++ gcc/testsuite/gcc.dg/pr31096-1.c(revision 242954)
@@ -2,8 +2,16 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fdump-tree-optimized" } */
 
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __INT32_TYPE__  int32_t;
+  __extension__ typedef __UINT32_TYPE__ uint32_t;
+#else
+  typedef int int32_t;
+  typedef unsigned uint32_t;
+#endif
+
 #define zero(name, op) \
-int name (int a, int b) \
+int32_t name (int32_t a, int32_t b) \
 { return a * 0 op b * 0; }
 
 zero(zeq, ==) zero(zne, !=) zero(zlt, <)
@@ -10,7 +18,7 @@
 zero(zgt, >)  zero(zge, >=) zero(zle, <=)
 
 #define unsign_pos(name, op) \
-int name (unsigned a, unsigned b) \
+int32_t name (uint32_t a, uint32_t b) \
 { return a * 4 op b * 4; }
 
 unsign_pos(upeq, ==) unsign_pos(upne, !=) unsign_pos(uplt, <)
@@ -17,7 +25,7 @@
 unsign_pos(upgt, >)  unsign_pos(upge, >=) unsign_pos(uple, <=)
 
 #define unsign_neg(name, op) \
-int name (unsigned a, unsigned b) \
+int32_t name (uint32_t a, uint32_t b) \
 { return a * -2 op b * -2; }
 
 unsign_neg(uneq, ==) unsign_neg(unne, !=) unsign_neg(unlt, <)
@@ -24,7 +32,7 @@
 unsign_neg(ungt, >)  unsign_neg(unge, >=) unsign_neg(unle, <=)
 
 #define float(name, op) \
-int name (float a, float b) \
+int32_t name (float a, float b) \
 { return a * 5 op b * 5; }
 
 float(feq, ==) float(fne, !=) float(flt, <)
@@ -31,7 +39,7 @@
 float(fgt, >)  float(fge, >=) float(fle, <=)
 
 #define float_val(name, op) \
-int name (int a, int b) \
+int32_t name (int32_t a, int32_t b) \
 { return a * 54.0 op b * 54.0; }
 
 float_val(fveq, ==) float_val(fvne, !=) float_val(fvlt, <)
@@ -38,8 +46,8 @@
 float_val(fvgt, >)  float_val(fvge, >=) float_val(fvle, <=)
 
 #define vec(name, op) \
-int name (int a, int b) \
-{ int c[10]; return a * c[1] op b * c[1]; }
+int32_t name (int32_t a, int32_t b) \
+{ int32_t c[10]; return a * c[1] op b * c[1]; }
 
 vec(veq, ==) vec(vne, !=) vec(vlt, <)
 vec(vgt, >)  vec(vge, >=) vec(vle, <=)

[Patch, testsuite] Fix bogus pr64427.c failure for avr

[Senthil Kumar Selvaraj]

  The smaller int size for the avr target breaks the test's
  expectation on the number of iterations. The failure goes
  away if 32 bit ints are used in place of a plain int.

  Fix by conditionally typedef int32_t to __INT32_TYPE__ for targets
  with int size < 4,  and then use int32_t everywhere.

Regards
Senthil

2016-11-25  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/pr64277.c: Use 32 bit int for targets
with sizeof(int) < 4. 

Index: gcc/testsuite/gcc.dg/pr64277.c
===
--- gcc/testsuite/gcc.dg/pr64277.c  (revision 242857)
+++ gcc/testsuite/gcc.dg/pr64277.c  (working copy)
@@ -4,10 +4,16 @@
 /* { dg-final { scan-tree-dump "loop with 5 iterations completely unrolled" 
"cunroll" } } */
 /* { dg-final { scan-tree-dump "loop with 6 iterations completely unrolled" 
"cunroll" } } */
 
-int f1[10];
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __INT32_TYPE__ int32_t;
+#else
+  typedef int int32_t;
+#endif
+
+int32_t f1[10];
 void test1 (short a[], short m, unsigned short l)
 {
-  int i = l;
+  int32_t i = l;
   for (i = i + 5; i < m; i++)
 f1[i] = a[i]++;
 }
@@ -14,7 +20,7 @@
 
 void test2 (short a[], short m, short l)
 {
-  int i;
+  int32_t i;
   if (m > 5)
 m = 5;
   for (i = m; i > l; i--)

Re: [Patch, tentative] Use MOVE_MAX_PIECES instead of MOVE_MAX in gimple_fold_builtin_memory_op

[Senthil Kumar Selvaraj]

Richard Biener writes:

> On Thu, 24 Nov 2016, Richard Biener wrote:
>
>> On Thu, 24 Nov 2016, Senthil Kumar Selvaraj wrote:
>> 
>> > Hi,
>> > 
>> >   I've been analyzing a failing regtest (gcc.dg/strlenopt-8.c) for the avr
>> >   target. I found that the (dump) failure is because there are 4
>> >   instances of memcpy, while the testcase expects only 2 for a
>> >   non-strict align target like the avr.
>> > 
>> >   Comparing that with a dump generated by x64_64-pc-linux, I found that
>> >   the extra memcpy's come from the forwprop pass, when it replaces
>> >   strcat with strlen and memcpy. For x86_64, the memcpy generated gets
>> >   folded into a load/store in gimple_fold_builtin_memory_op. That
>> >   doesn't happen for the avr because len (2) happens to be bigger than
>> >   MOVE_MAX (1).
>> > 
>> >   The avr can only move 1 byte efficiently from reg <-> memory, but it's
>> >   more efficient to load and store 2 bytes than to call memcpy, so
>> >   MOVE_MAX_PIECES is set to 2.
>> > 
>> >   Given that gimple_fold_builtin_memory_op gets to choose between
>> >   leaving the memcpy call as is, or breaking it down to a by-pieces
>> >   move, shouldn't it use MOVE_MAX_PIECES instead of
>> >   MOV_MAX?
>> > 
>> >   That is what the below patch does, and that makes the test
>> >   pass. Does this sound right?
>> 
>> No, as we handle both memcpy and memmove this way we rely on
>> the whole storage fit in a single register so we do the
>> right thing for overlapping memory.
>
> So actually your patch doesn't chnage that, the ordering is ensured
> by emitting a single GIMPLE load/store pair.  There are only
> four targets defining MOVE_MAX_PIECES, and one (s390) even has
> a smaller MOVE_MAX_PIECES than MOVE_MAX (huh).  AVR has larger
> MOVE_MAX_PIECES than MOVE_MAX, but that seems to not make much
> sense to me given their very similar description plus the
> fact that AVR can only load a single byte at a time...
>
> The x86 comment says
>
> /* MOVE_MAX_PIECES is the number of bytes at a time which we can
>move efficiently, as opposed to  MOVE_MAX which is the maximum
>number of bytes we can move with a single instruction.
>
> which doesn't match up with
>
> @defmac MOVE_MAX
> The maximum number of bytes that a single instruction can move quickly
> between memory and registers or between two memory locations.
> @end defmac
>
> note "quickly" here.  But OTOH
>
> @defmac MOVE_MAX_PIECES
> A C expression used by @code{move_by_pieces} to determine the largest unit
> a load or store used to copy memory is.  Defaults to @code{MOVE_MAX}.
> @end defmac
>
> here the only difference is "copy memory".  But we try to special
> case the one load - one store case, not generally "copy memory".
>
> So I think MOVE_MAX matches my intent when writing the code.

Ok, but isn't that inconsistent with tree-inline.c:estimate_move_cost, which
considers MOVE_MAX_PIECES and MOVE_RATIO to decide between a libcall and
by-pieces move?

Regards
Senthil

>
> Richard.
>
>> Richard.
>> 
>> > Regards
>> > Senthil
>> > 
>> > Index: gcc/gimple-fold.c
>> > ===
>> > --- gcc/gimple-fold.c  (revision 242741)
>> > +++ gcc/gimple-fold.c  (working copy)
>> > @@ -703,7 +703,7 @@
>> >src_align = get_pointer_alignment (src);
>> >dest_align = get_pointer_alignment (dest);
>> >if (tree_fits_uhwi_p (len)
>> > -&& compare_tree_int (len, MOVE_MAX) <= 0
>> > +&& compare_tree_int (len, MOVE_MAX_PIECES) <= 0
>> >  /* ???  Don't transform copies from strings with known length this
>> > confuses the tree-ssa-strlen.c.  This doesn't handle
>> > the case in gcc.dg/strlenopt-8.c which is XFAILed for that
>> > 
>> > 
>> 
>> 

[Patch, tentative] Use MOVE_MAX_PIECES instead of MOVE_MAX in gimple_fold_builtin_memory_op

[Senthil Kumar Selvaraj]

Hi,

  I've been analyzing a failing regtest (gcc.dg/strlenopt-8.c) for the avr
  target. I found that the (dump) failure is because there are 4
  instances of memcpy, while the testcase expects only 2 for a
  non-strict align target like the avr.

  Comparing that with a dump generated by x64_64-pc-linux, I found that
  the extra memcpy's come from the forwprop pass, when it replaces
  strcat with strlen and memcpy. For x86_64, the memcpy generated gets
  folded into a load/store in gimple_fold_builtin_memory_op. That
  doesn't happen for the avr because len (2) happens to be bigger than
  MOVE_MAX (1).

  The avr can only move 1 byte efficiently from reg <-> memory, but it's
  more efficient to load and store 2 bytes than to call memcpy, so
  MOVE_MAX_PIECES is set to 2.

  Given that gimple_fold_builtin_memory_op gets to choose between
  leaving the memcpy call as is, or breaking it down to a by-pieces
  move, shouldn't it use MOVE_MAX_PIECES instead of
  MOV_MAX?

  That is what the below patch does, and that makes the test
  pass. Does this sound right?

Regards
Senthil

Index: gcc/gimple-fold.c
===
--- gcc/gimple-fold.c   (revision 242741)
+++ gcc/gimple-fold.c   (working copy)
@@ -703,7 +703,7 @@
   src_align = get_pointer_alignment (src);
   dest_align = get_pointer_alignment (dest);
   if (tree_fits_uhwi_p (len)
- && compare_tree_int (len, MOVE_MAX) <= 0
+ && compare_tree_int (len, MOVE_MAX_PIECES) <= 0
  /* ???  Don't transform copies from strings with known length this
 confuses the tree-ssa-strlen.c.  This doesn't handle
 the case in gcc.dg/strlenopt-8.c which is XFAILed for that

Re: [Patch, testsuite] Fix bogus uninit-19.c failure for avr

[Senthil Kumar Selvaraj]

Jeff Law writes:

> On 11/23/2016 02:54 AM, Senthil Kumar Selvaraj wrote:
>> Hi,
>>
>>   The below patch fixes uninit-19.c for avr by adding
>>   -finline-small-functions for avr.
>>
>>   The test fails for avr because fn1 does not get inlined into fn2. Inlining
>>   occurs for x86_64 because fn1's computed size equals call_stmt_size. For
>>   avr, 32 bit memory moves are more expensive, and b[3] = p10[a] results in
>>   a bigger size for fn1, resulting in estimate_growth > 0 and no inlining.
>>
>>   Adding -finline-small-functions forces early inliner to inline fn1,
>>   making the test pass.
>>
>>   Committed to trunk.
>>
>> Regards
>> Senthil
>>
>> gcc/testsuite/ChangeLog
>>
>> 2016-11-23  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>
>> * gcc.dg/uninit-19.c: Add -finline-small-functions for avr.
> How about instead forcing inlining via the always_inline attribute?  If 
> inlining is indeed expected and necessary, that seems better than 
> -finline-small-functions.

I considered it, but a previous discussion in the mailing list backed
off from a similar proposal
(https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00517.html), and I wasn't
sure if always_inline would break the pic/non-pic distinction somehow.

Should I revert this and mark the function always_inline/static?

>
> Jeff

[Patch, testsuite] Fix bogus uninit-19.c failure for avr

[Senthil Kumar Selvaraj]

Hi,

  The below patch fixes uninit-19.c for avr by adding
  -finline-small-functions for avr.

  The test fails for avr because fn1 does not get inlined into fn2. Inlining
  occurs for x86_64 because fn1's computed size equals call_stmt_size. For
  avr, 32 bit memory moves are more expensive, and b[3] = p10[a] results in 
  a bigger size for fn1, resulting in estimate_growth > 0 and no inlining.

  Adding -finline-small-functions forces early inliner to inline fn1,
  making the test pass.

  Committed to trunk.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-11-23  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/uninit-19.c: Add -finline-small-functions for avr.


Index: testsuite/gcc.dg/uninit-19.c
===
--- testsuite/gcc.dg/uninit-19.c(revision 242741)
+++ testsuite/gcc.dg/uninit-19.c(working copy)
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O -Wuninitialized" } */
+/* { dg-additional-options "-finline-small-functions" { target avr-*-* } } */
 
 int a, l, m;
 float *b;
@@ -10,7 +11,7 @@
  unsigned char *c2, float *p10)
 {
   if (p1 & 8)
-b[3] = p10[a];  /* 13.  */
+b[3] = p10[a];  /* 14.  */
 }
 
 void
@@ -19,8 +20,8 @@
   float *n;
   if (l & 6)
 n =  + m;
-  fn1 (l, , , , , , , n);  /* 22.  */
+  fn1 (l, , , , , , , n);  /* 23.  */
 }
 
-/* { dg-warning "may be used uninitialized" "" { target { { nonpic } || { 
hppa*64*-*-* } } } 13 } */
-/* { dg-warning "may be used uninitialized" "" { target { ! { { nonpic } || { 
hppa*64*-*-* } } } } 22 } */
+/* { dg-warning "may be used uninitialized" "" { target { { nonpic } || { 
hppa*64*-*-* } } } 14 } */
+/* { dg-warning "may be used uninitialized" "" { target { ! { { nonpic } || { 
hppa*64*-*-* } } } } 23 } */

Re: [patch,testsuite] Support dg-require-effective-target label_offsets.

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> On 04.11.2016 06:18, Senthil Kumar Selvaraj wrote:
>>
>> Georg-Johann Lay writes:
>>> State of matters is that Binutils support is missing, and if I understand 
>>> you
>>> correctly, dg-require is not appropriate to factor out availability of such
>>> features?
>>
>> I'll take a stab at adding the missing binutils support for avr label diffs.
>
> Thanks for taking care of this.  I had a look into it but got stuck with a 
> test 
> case derived from gcc.c-torture/execute/pr70460.c
>
> int c;
>
> __attribute__((noinline, noclone)) void
> call (void)
> {
>__asm ("nop");
> }
>
> __attribute__((noinline, noclone)) void
> foo (int x)
> {
>static int b[] = { & - &, & - & };
>void *a = & + b[x];
>goto *a;
> lab1:
>c += 2;
>call();
> lab2:
>c++;
> lab0:
>;
> }
>
> int
> main ()
> {
>foo (0);
>if (c != 3)
>  __builtin_abort ();
>foo (1);
>if (c != 4)
>  __builtin_abort ();
>return 0;
> }
>
>
> The problem is when relaxation is turned on and the CALL is shortened to  
> RCALL 
> but respective literals are not fixed up.

That linker issue is fixed with 
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=4cb771f214ed6a2102e37bce255c6be5d0642f3a

Regards
Senthil

[Patch, testsuite] Fix bogus Wlogical-op-1.c test failure for avr

[Senthil Kumar Selvaraj]

Hi,

  The below patch fixes c-c++-common/Wlogical-op-1.c for avr by
  explicitly typedef'ing __INT32_TYPE for int and __INT16_TYPE__ for short
  if the target's int size is less than 4 bytes.

  The test assumes short is always smaller than int, and therefore does not 
  expect a warning when the logical operands are of type short and int.

  This isn't true for the avr - shorts and ints are of the same size, and
  therefore the warning triggers for the above case also.

  Committed to trunk as obvious.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-11-16  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* c-c++-common/Wlogical-op-1.c: Use __INT{16,32}_TYPE__ instead
of {short,int} if __SIZEOF_INT__ is less than 4 bytes.


Index: c-c++-common/Wlogical-op-1.c
===
--- c-c++-common/Wlogical-op-1.c(revision 242471)
+++ c-c++-common/Wlogical-op-1.c(working copy)
@@ -8,12 +8,22 @@
 # define false 0
 #endif
 
-extern int bar (void);
-extern int *p;
-struct R { int a, b; } S;
+#if __SIZEOF_INT__ < 4
+  __extension__ typedef __INT32_TYPE__ int32_t;
+  __extension__ typedef __UINT32_TYPE__ uint32_t;
+  __extension__ typedef __INT16_TYPE__ int16_t;
+#else
+  typedef int int32_t;
+  typedef unsigned int uint32_t;
+  typedef short int16_t;
+#endif
 
+extern int32_t bar (void);
+extern int32_t *p;
+struct R { int32_t a, b; } S;
+
 void
-andfn (int a, int b)
+andfn (int32_t a, int32_t b)
 {
   if (a && a) {}   /* { dg-warning "logical .and. of equal 
expressions" } */
   if (!a && !a) {} /* { dg-warning "logical .and. of equal 
expressions" } */
@@ -34,7 +44,7 @@
   if (p[0] && p[0]) {} /* { dg-warning "logical .and. of equal 
expressions" } */
   if (S.a && S.a) {}   /* { dg-warning "logical .and. of equal 
expressions" } */
   if ((bool) a && (bool) a) {} /* { dg-warning "logical .and. of equal 
expressions" } */
-  if ((unsigned) a && a) {}/* { dg-warning "logical .and. of equal 
expressions" } */
+  if ((uint32_t) a && a) {}/* { dg-warning "logical .and. of equal 
expressions" } */
 
   /* Stay quiet here.  */
   if (a && b) {}
@@ -48,7 +58,7 @@
 
   if (a > 0 && a > 1) {}
   if (a > -2 && a > 1) {}
-  if (a && (short) a) {}
+  if (a && (int16_t) a) {}
   if ((char) a && a) {}
   if (++a && a) {}
   if (++a && ++a) {}
@@ -61,7 +71,7 @@
 }
 
 void
-orfn (int a, int b)
+orfn (int32_t a, int32_t b)
 {
   if (a || a) {}   /* { dg-warning "logical .or. of equal 
expressions" } */
   if (!a || !a) {} /* { dg-warning "logical .or. of equal 
expressions" } */
@@ -82,7 +92,7 @@
   if (p[0] || p[0]) {} /* { dg-warning "logical .or. of equal 
expressions" } */
   if (S.a || S.a) {}   /* { dg-warning "logical .or. of equal 
expressions" } */
   if ((bool) a || (bool) a) {} /* { dg-warning "logical .or. of equal 
expressions" } */
-  if ((unsigned) a || a) {}/* { dg-warning "logical .or. of equal 
expressions" } */
+  if ((uint32_t) a || a) {}/* { dg-warning "logical .or. of equal 
expressions" } */
 
   /* Stay quiet here.  */
   if (a || b) {}
@@ -96,7 +106,7 @@
 
   if (a > 0 || a > 1) {}
   if (a > -2 || a > 1) {}
-  if (a || (short) a) {}
+  if (a || (int16_t) a) {}
   if ((char) a || a) {}
   if (++a || a) {}
   if (++a || ++a) {}

Re: [patch,testsuite] Support dg-require-effective-target label_offsets.

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> On 28.10.2016 17:58, Mike Stump wrote:
>> On Oct 27, 2016, at 3:16 AM, Georg-Johann Lay  wrote:
>>>
>>> Now imagine some arithmetic like & - &  This might result in
>>> one or two stub addresses, and difference between such addresses is a
>>> complete different thing than the difference between the original labels:
>>> The result might differ in absolute value and in sign, i.e. you can't even
>>> determine whether LAB1 or LAB2 comes first in the generated code as the
>>> order of stubs might differ from the order of respective labels.
>>
>> So, I think this all doesn't matter any.  Every address gs(LAB) fits in
>> 16-bits by definition, and every gs(LAB1) - gs(LAB2) fits into 16 bits and
>
> Yes, you are right.  Label differences could be implemented.  AFAIK there is 
> currently no activity by the Binutils folks to add respective support, so it 
> is 
> somewhat pointless to add that support to avr-gcc...
>
> Bottom line is that label offsets are not supported by avr BE, and the 
> intention of the patch is to reduce FAIL noise in testsuite results because 
> of 
> the missing support.
>
> If a dg-require is not appropriate, should I rather add dg-skip-if to every 
> test case?  I'd still prefer the dg-require solution because if the Binutils 
> will ever support label differences, then the testsuite will automatically 
> catch up with that.
>
>> thus is valid for all 16-bit one contexts.  The fact the order between the
>> stub and the actual code is different is irrelevant, it is a private
>
> Only if the code is not executed; there are several test cases that compute 
> relative placements of labels like:
>
> x(){if(&&<0)x();b:goto*&e:;}
>
> If a test ever relies on the fact that & - & tells anything about the 
> ordering of the labels, the code is about to crash.
>
>> implementation detail of the port, the point is the semantics are fixed and
>> constant and useful.  In deed that there is even a stub is a private
>> implementation detail of the port.  I think the `extra' helpful warning from
>> avr_print_operand_address is wrong and should be removed.  Think of the
>
> The following code simple makes absolutely no sense in the presence of stubs:
> int
> test (int foo)
> {
>static void *dummy[] = { &, & };
>goto *((char *) & - 2 * (foo < 0));
> a:
> b:
>return 0;
> }
>
> It's only a compile test (the original PR66123 is about ICE), but the 
> compiler 
> cannot know that it's just a crazy test case that won't be executed...
>
> When you are offsetting from a stub you might end up *anywhere*, even in some 
> data range.
>
>> label as gs(LAB), not LAB, burn LAB from your mind.  Once you do that, you
>> see you can't talk about the order LAB1 > LAB2, the concept doesn't make any
>> sense.  The _only_ think you can talk about is gs(LAB1) > gs(LAB2).  And
>> because of that, it is always consistent and works just fine.
>>
>> Once that misguided complains from gcc and bisutils are fixed, are their any
>> failing cases?
>
> State of matters is that Binutils support is missing, and if I understand you 
> correctly, dg-require is not appropriate to factor out availability of such 
> features?

I'll take a stab at adding the missing binutils support for avr label diffs.

Regards
Senthil

[Patch, testsuite] Add new effective-target_store_merge

[Senthil Kumar Selvaraj]

Hi,

  The below patch adds a new effective target keyword (store_merge) for
  use in the store_merging_xxx.c tests.

  The tests currently require non_strict_align, but they still fail for the avr.
  Eyeballing the dump, I found that the pass doesn't attempt merging as it is
  unprofitable for a target like the avr which has only single byte
  stores.

  I figured store merging is unlikely to be profitable for targets with
  smallish word sizes, and added a check_effective_target_store_merge
  that combines non_strict_align and int32plus.

  Is this ok for trunk?

Regards
Senthil

gcc/testsuite/ChangeLog

2016-11-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/store_merging_1.c: Require store_merge.
* gcc.dg/store_merging_2.c: Likewise.
* gcc.dg/store_merging_4.c: Likewise.
* gcc.dg/store_merging_5.c: Likewise. 
* gcc.dg/store_merging_6.c: Likewise.
* gcc.dg/store_merging_7.c: Likewise.
* gcc.dg/store_merging_8.c: Likewise.
* lib/target-supports.exp (check_effective_target_store_merge): New.


Index: gcc/testsuite/gcc.dg/store_merging_1.c
===
--- gcc/testsuite/gcc.dg/store_merging_1.c  (revision 241808)
+++ gcc/testsuite/gcc.dg/store_merging_1.c  (working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-require-effective-target non_strict_align } */
+/* { dg-require-effective-target store_merge } */
 /* { dg-options "-O2 -fdump-tree-store-merging" } */
 
 struct bar {
Index: gcc/testsuite/gcc.dg/store_merging_2.c
===
--- gcc/testsuite/gcc.dg/store_merging_2.c  (revision 241808)
+++ gcc/testsuite/gcc.dg/store_merging_2.c  (working copy)
@@ -1,5 +1,5 @@
 /* { dg-do run } */
-/* { dg-require-effective-target non_strict_align } */
+/* { dg-require-effective-target store_merge } */
 /* { dg-options "-O2 -fdump-tree-store-merging" } */
 
 struct bar
Index: gcc/testsuite/gcc.dg/store_merging_4.c
===
--- gcc/testsuite/gcc.dg/store_merging_4.c  (revision 241808)
+++ gcc/testsuite/gcc.dg/store_merging_4.c  (working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-require-effective-target non_strict_align } */
+/* { dg-require-effective-target store_merge } */
 /* { dg-options "-O2 -fdump-tree-store-merging" } */
 
 /* Check that we can merge interleaving stores that are guaranteed
Index: gcc/testsuite/gcc.dg/store_merging_5.c
===
--- gcc/testsuite/gcc.dg/store_merging_5.c  (revision 241808)
+++ gcc/testsuite/gcc.dg/store_merging_5.c  (working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-require-effective-target non_strict_align } */
+/* { dg-require-effective-target store_merge } */
 /* { dg-options "-O2 -fdump-tree-store-merging" } */
 
 /* Make sure that non-aliasing non-constant interspersed stores do not
Index: gcc/testsuite/gcc.dg/store_merging_6.c
===
--- gcc/testsuite/gcc.dg/store_merging_6.c  (revision 241808)
+++ gcc/testsuite/gcc.dg/store_merging_6.c  (working copy)
@@ -1,5 +1,5 @@
 /* { dg-do run } */
-/* { dg-require-effective-target non_strict_align } */
+/* { dg-require-effective-target store_merge } */
 /* { dg-options "-O2 -fdump-tree-store-merging" } */
 
 /* Check that we can widen accesses to bitfields.  */
Index: gcc/testsuite/gcc.dg/store_merging_7.c
===
--- gcc/testsuite/gcc.dg/store_merging_7.c  (revision 241808)
+++ gcc/testsuite/gcc.dg/store_merging_7.c  (working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-require-effective-target non_strict_align } */
+/* { dg-require-effective-target store_merge } */
 /* { dg-options "-O2 -fdump-tree-store-merging" } */
 
 /* Check that we can merge consecutive array members through the pointer.
Index: gcc/testsuite/gcc.dg/store_merging_8.c
===
--- gcc/testsuite/gcc.dg/store_merging_8.c  (revision 241808)
+++ gcc/testsuite/gcc.dg/store_merging_8.c  (working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-require-effective-target non_strict_align } */
+/* { dg-require-effective-target store_merge } */
 /* { dg-options "-O2 -fdump-tree-store-merging" } */
 
 struct baz {
Index: gcc/testsuite/lib/target-supports.exp
===
--- gcc/testsuite/lib/target-supports.exp   (revision 241808)
+++ gcc/testsuite/lib/target-supports.exp   (working copy)
@@ -8107,3 +8107,16 @@
 
 return [check_effective_target_divmod]
 }
+
+# Return 1 if store merging optimization is applicable for target.
+# Stor

[Patch, testsuite] Fix bogus PR 78170 failure for avr

[Senthil Kumar Selvaraj]

Hi,

  The below patch requires int32plus for
  gcc.c-torture/execute/pr78170.c, as it has int bitfields that are more
  than 16 bits wide.

  Committed to trunk as obvious.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-11-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.c-torture/execute/pr78170.c: Require int32plus.



Index: gcc.c-torture/execute/pr78170.c
===
--- gcc.c-torture/execute/pr78170.c (revision 241808)
+++ gcc.c-torture/execute/pr78170.c (working copy)
@@ -1,3 +1,5 @@
+/* { dg-require-effective-target int32plus } */
+
 /* PR tree-optimization/78170.
Check that sign-extended store to a bitfield
doesn't overwrite other fields.  */

[Patch, testsuite] Skip gcc.dg/lto/pr60449_0.c for avr

[Senthil Kumar Selvaraj]

Hi,

  gcc.dg/lto/pr60449_0.c fails to link for the avr target because it
  doesn't have an implementation for gettimeofday. This patch therefore
  skips the test for avr.

  Committed to trunk as obvious.

Regards
Senthil


gcc/testsuite/ChangeLog

2016-10-31  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/lto/pr60449_0.c: Skip for avr.

Index: gcc/testsuite/gcc.dg/lto/pr60449_0.c
===
--- gcc/testsuite/gcc.dg/lto/pr60449_0.c(revision 241697)
+++ gcc/testsuite/gcc.dg/lto/pr60449_0.c(working copy)
@@ -1,4 +1,5 @@
 /* { dg-lto-do link } */
+/* { dg-skip-if "Needs gettimeofday" { "avr-*-*" } } */
 
 extern int printf (const char *__restrict __format, ...);
 typedef long int __time_t;

 

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Bernd Schmidt writes:

> On 10/18/2016 02:15 PM, Senthil Kumar Selvaraj wrote:
>> Will do both the changes and re-run the reg tests. Ok for trunk if the
>> tests pass for x86_64-pc-linux and avr?
>>
> Probably but let's see the patch first.

How does this look?

Bootstrapped and reg tested x86_64-pc-linux on top of trunk@190252 with
the in_hard_reg_set_p patch backport - there were no failures. Also ran
regtests for avr on trunk, no failures there as well.

Ok to commit to trunk?

Regards
Senthil

gcc/ChangeLog:

2016-10-21  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* reload.c (find_valid_class_1): Allow regclass if atleast one
regno in class is ok. Compute and use rclass size based on
actually available regnos for mode in rclass.

gcc/testsuite/ChangeLog:

2016-10-21  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.target/avr/pr71627.c: New test.




diff --git gcc/reload.c gcc/reload.c
index 9a859e5..880099e 100644
--- gcc/reload.c
+++ gcc/reload.c
@@ -715,25 +715,23 @@ find_valid_class_1 (machine_mode outer ATTRIBUTE_UNUSED,
 
   for (rclass = 1; rclass < N_REG_CLASSES; rclass++)
 {
-  int bad = 0;
-  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++)
-   {
- if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
- && !HARD_REGNO_MODE_OK (regno, mode))
-   bad = 1;
-   }
-  
-  if (bad)
-   continue;
+  unsigned int computed_rclass_size = 0;
+
+  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
+{
+  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
+  && (HARD_REGNO_MODE_OK (regno, mode)))
+computed_rclass_size++;
+}
 
   cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
 
-  if ((reg_class_size[rclass] > best_size
+  if ((computed_rclass_size > best_size
   && (best_cost < 0 || best_cost >= cost))
  || best_cost > cost)
{
  best_class = (enum reg_class) rclass;
- best_size = reg_class_size[rclass];
+ best_size = computed_rclass_size;
  best_cost = register_move_cost (outer, (enum reg_class) rclass,
  dest_class);
}
diff --git gcc/testsuite/gcc.target/avr/pr71627.c 
gcc/testsuite/gcc.target/avr/pr71627.c
new file mode 100644
index 000..eaef3d2
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71627.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+
+extern volatile __memx const long  a, b, c, d, e, f;
+extern volatile long result;
+
+extern void vfunc (const char*, ...);
+
+void foo (void)
+{
+   result = a + b + c + d + e + f;
+   vfunc ("text", a, b, c, d, e, f, result);
+}

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Bernd Schmidt writes:

> On 10/13/2016 08:57 AM, Senthil Kumar Selvaraj wrote:
>>
>> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>
>>  * reload.c (find_valid_class_1): Allow regclass if atleast one
>>  regno in class is ok. Compute and use rclass size based on
>>  actually available regnos for mode in rclass.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>  
>>  * gcc.target/avr/pr71627.c: New.
>>
>>
>> Index: gcc/reload.c
>> ===
>> --- gcc/reload.c (revision 240989)
>> +++ gcc/reload.c (working copy)
>> @@ -711,31 +711,36 @@
>>enum reg_class best_class = NO_REGS;
>>unsigned int best_size = 0;
>>int cost;
>> +  unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 };
>
> As far as I can tell you're only accessing this as 
> computed_rclass_size[rclass], i.e. with the current class in the loop. 
> So I don't think you need the array at all, just a computed_size 
> variable in the loop?

Yes - I mechanically replaced the original array with the computed one.
A variable would suffice.
>
>> +  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
>> +{
>> +  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
>> +{
>> +  atleast_one_regno_ok = 1;
>> +  if (HARD_REGNO_MODE_OK (regno, mode))
>> +computed_rclass_sizes[rclass]++;
>> +}
>> +}
>
> Don't you want to also ensure HARD_REGNO_MODE_OK before claiming that 
> atleast_one_regno_ok? Maybe I'm forgetting the motivation but this seems 
> odd. If so, the variable becomes unnecessary, just check the computed size.

True again - the original intention was to prevent the best_xxx
variables from getting set if no regno was in_hard_reg_set. Now the
computed class size would be zero, so the variable is unnecessary.

Will do both the changes and re-run the reg tests. Ok for trunk if the
tests pass for x86_64-pc-linux and avr?

Regards
Senthil

[Patch,testsuite] Fix sso.exp not calling torture-finish for avr

[Senthil Kumar Selvaraj]

Hi,

  When analyzing reg test failures for the avr target, I noticed that the
  torture options were different when running dg-torture.exp compared to
  x86_64-pc-linux-gnu, resulting in additional failures. I also found
  that  a bunch of "torture-without-loops not empty as expected" errors
  show up for a few .exp files.

  I found that these did not occur when the exp files were run in
  isolation. On further debugging, I found that sso.exp calls dg-init and
  torture-init, and returns if !effective_target_int32. It does
  not call the corresponding finish functions for targets like the avr
  for which the effective target condition is true, and this leaves
  torture-options set, which causes the errors and differing options.

  The below patch makes the return occur earlier - before calling the
  init functions.

  Committed to trunk.

Regards
Senthil

2016-10-18  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/sso/sso.exp: Return early if not
effective_target_int32.


Index: gcc.dg/sso/sso.exp
===
--- gcc.dg/sso/sso.exp  (revision 241299)
+++ gcc.dg/sso/sso.exp  (working copy)
@@ -18,6 +18,10 @@
 load_lib gcc-dg.exp
 load_lib torture-options.exp
 
+if { ![check_effective_target_int32] } {
+return
+}
+
 # Initialize `dg'.
 torture-init
 dg-init
@@ -32,10 +36,6 @@
 
 set-torture-options $SSO_TORTURE_OPTIONS
 
-if { ![check_effective_target_int32] } {
-return
-}
-
 # Main loop.
 gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] "" ""
 

Re: [Patch] Backport fix for PR 52085 to gcc-5-branch?

[Senthil Kumar Selvaraj]

Jakub Jelinek writes:

> On Tue, Oct 18, 2016 at 02:46:29PM +0530, Senthil Kumar Selvaraj wrote:
>> > I'm not convinced it is desirable to backport such changes, it affects ABI,
>> > people are used to deal with minor ABI changes in between major GCC
>> > releases, but we'd need a strong reason to change it between minor 
>> > releases.
>> 
>> Hmm, I tracked this down from a (internal) bug reported on arm-none-eabi, 
>> where the
>> inconsistent enum size (used in sizeof to malloc) was eventually causing
>> heap corruption. 
>> 
>> When debugging the issue, I noticed you'd already backported
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69669, so I thought this
>> should be good.
>
> That one has been a regression, older GCCs handled it the same as does the 5
> branch now.  Is that the case here?

No, I can reproduce the bug on 4.9 as well. So not ok to backport then, I
guess?

Regards
Senthil

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Ping!

Regards
Senthil

Senthil Kumar Selvaraj writes:

> Bernd Schmidt writes:
>
>> On 09/16/2016 09:02 PM, Senthil Kumar Selvaraj wrote:
>>>   Does this make sense? I ran a reg test for the avr target with a
>>>   slightly older version of this patch, it did not show any regressions.
>>>   If this is the right fix, I'll make sure to run reg tests on x86_64
>>>   after backporting to a gcc version where that target used reload.
>>
>> It's hard to say, and could have different effects on different targets.
>> One thing though, at the very least the reg_class_size test would have 
>> to be adapted - the idea is to find the largest class, and there's a 
>> risk here of ending up with a large class that only has one valid register.
>
> Agreed - I've updated the patch to compute rclass sizes based on regno
> availability i.e., only if in_hard_reg_set_p and HARD_REGNO_MODE_OK, and
> then use the computed sizes when calculating best_size.
>
>>
>> You'll also want to verify this against
>>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54814
>
> Yes, this patch doesn't break the fix for PR54814. The change to
> in_hard_reg_set_p was what fixed that, and that remains unmodified.
>
> Reg tested this on top of trunk@190252 with the in_hard_reg_set_p
> backport. x86_64-pc-linux bootstrapped and regtested ok. avr showed
> no regressions either.
>
> Ok for trunk?
>
> Regards
> Senthil
>
> gcc/ChangeLog:
>
> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>
>   * reload.c (find_valid_class_1): Allow regclass if atleast one
>   regno in class is ok. Compute and use rclass size based on
>   actually available regnos for mode in rclass.
>
> gcc/testsuite/ChangeLog:
>
> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>   
>   * gcc.target/avr/pr71627.c: New.
>
>
> Index: gcc/reload.c
> ===
> --- gcc/reload.c  (revision 240989)
> +++ gcc/reload.c  (working copy)
> @@ -711,31 +711,36 @@
>enum reg_class best_class = NO_REGS;
>unsigned int best_size = 0;
>int cost;
> +  unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 };
>  
>for (rclass = 1; rclass < N_REG_CLASSES; rclass++)
>  {
> -  int bad = 0;
> -  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++)
> - {
> -   if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
> -   && !HARD_REGNO_MODE_OK (regno, mode))
> - bad = 1;
> - }
> -  
> -  if (bad)
> - continue;
> +  int atleast_one_regno_ok = 0;
>  
> +  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
> +{
> +  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
> +{
> +  atleast_one_regno_ok = 1;
> +  if (HARD_REGNO_MODE_OK (regno, mode))
> +computed_rclass_sizes[rclass]++;
> +}
> +}
> +
> +  if (!atleast_one_regno_ok)
> +continue;
> +
>cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
>  
> -  if ((reg_class_size[rclass] > best_size
> -&& (best_cost < 0 || best_cost >= cost))
> -   || best_cost > cost)
> - {
> -   best_class = (enum reg_class) rclass;
> -   best_size = reg_class_size[rclass];
> -   best_cost = register_move_cost (outer, (enum reg_class) rclass,
> -   dest_class);
> - }
> +  if ((computed_rclass_sizes[rclass] > best_size
> + && (best_cost < 0 || best_cost >= cost))
> +|| best_cost > cost)
> +  {
> +best_class = (enum reg_class) rclass;
> +best_size = computed_rclass_sizes[rclass];
> +best_cost = register_move_cost (outer, (enum reg_class) rclass,
> +dest_class);
> +  }
>  }
>  
>gcc_assert (best_size != 0);
>
> Index: gcc/testsuite/gcc.target/avr/pr71627.c
> ===
> --- gcc/testsuite/gcc.target/avr/pr71627.c  (nonexistent)
> +++ gcc/testsuite/gcc.target/avr/pr71627.c  (working copy)
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1" } */
> +
> +
> +extern volatile __memx const long  a, b, c, d, e, f;
> +extern volatile long result;
> +
> +extern void vfunc (const char*, ...);
> +
> +void foo (void)
> +{
> +   result = a + b + c + d + e + f;
> +   vfunc ("text", a, b, c, d, e, f, result);
> +}

Re: [Patch] Backport fix for PR 52085 to gcc-5-branch?

[Senthil Kumar Selvaraj]

Jakub Jelinek writes:

> On Tue, Oct 18, 2016 at 10:12:24AM +0200, Richard Biener wrote:
>> On Mon, Oct 17, 2016 at 6:57 PM, Senthil Kumar Selvaraj
>> <senthil_kumar.selva...@atmel.com> wrote:
>> >
>> > Richard Biener writes:
>> >
>> >> On Mon, Oct 17, 2016 at 12:21 PM, Senthil Kumar Selvaraj
>> >> <senthil_kumar.selva...@atmel.com> wrote:
>> >>> Hi,
>> >>>
>> >>>   The fix for PR 52085 went into trunk when trunk was 6.0. I ran into the
>> >>>   same issue on a gcc 5.x and found that the fix didn't get backported.
>> >>>
>> >>>   Bootstrapped and reg tested below patch with x86-64-pc-linux. Ok to
>> >>>   backport to gcc-5-branch?
>> >>
>> >> Ok with me but please double-check there was no fallout.
>> >
>> > I boostrapped and ran against x86_64-pc-linux again, just to be sure.
>> > No regressions.
>> 
>> I meant fallout only fixed with followup patches.  ISTR some in that area
>> but I might confuse it with another patch.  Marek might remember.
>
> I'm not convinced it is desirable to backport such changes, it affects ABI,
> people are used to deal with minor ABI changes in between major GCC
> releases, but we'd need a strong reason to change it between minor releases.

Hmm, I tracked this down from a (internal) bug reported on arm-none-eabi, where 
the
inconsistent enum size (used in sizeof to malloc) was eventually causing
heap corruption. 

When debugging the issue, I noticed you'd already backported
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69669, so I thought this
should be good.

Regards
Senthil

>
>> >>> 2016-10-17  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>> >>>
>> >>>   Backport from mainline
>> >>> 2015-04-25  Marek Polacek  <pola...@redhat.com>
>> >>> PR c/52085
>> >>> * c-decl.c (finish_enum): Copy over TYPE_ALIGN.  Also check for 
>> >>> "mode"
>> >>> attribute.
>> >>>
>> >>> gcc/testsuite/ChangeLog
>> >>> 2016-10-17  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>> >>>
>> >>> Backport from mainline
>> >>> 2015-04-25  Marek Polacek  <pola...@redhat.com>
>> >>> PR c/52085
>> >>> * gcc.dg/enum-incomplete-2.c: New test.
>> >>> * gcc.dg/enum-mode-1.c: New test.
>
>   Jakub

Re: [Patch] Backport fix for PR 52085 to gcc-5-branch?

[Senthil Kumar Selvaraj]

Richard Biener writes:

> On Mon, Oct 17, 2016 at 12:21 PM, Senthil Kumar Selvaraj
> <senthil_kumar.selva...@atmel.com> wrote:
>> Hi,
>>
>>   The fix for PR 52085 went into trunk when trunk was 6.0. I ran into the
>>   same issue on a gcc 5.x and found that the fix didn't get backported.
>>
>>   Bootstrapped and reg tested below patch with x86-64-pc-linux. Ok to
>>   backport to gcc-5-branch?
>
> Ok with me but please double-check there was no fallout.

I boostrapped and ran against x86_64-pc-linux again, just to be sure.
No regressions.

I'll run the reg tests against arm-none-eabi. Can I commit it if that
passes?

Regards
Senthil
>
> Richard.
>
>> Regards
>> Senthil
>>
>> gcc/c/ChangeLog
>>
>> 2016-10-17  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>
>>   Backport from mainline
>> 2015-04-25  Marek Polacek  <pola...@redhat.com>
>> PR c/52085
>> * c-decl.c (finish_enum): Copy over TYPE_ALIGN.  Also check for 
>> "mode"
>> attribute.
>>
>> gcc/testsuite/ChangeLog
>> 2016-10-17  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>
>> Backport from mainline
>> 2015-04-25  Marek Polacek  <pola...@redhat.com>
>> PR c/52085
>> * gcc.dg/enum-incomplete-2.c: New test.
>> * gcc.dg/enum-mode-1.c: New test.
>>
>>
>> diff --git gcc/c/c-decl.c gcc/c/c-decl.c
>> index d1e7444..c508e7f 100644
>> --- gcc/c/c-decl.c
>> +++ gcc/c/c-decl.c
>> @@ -8050,7 +8050,7 @@ finish_enum (tree enumtype, tree values, tree 
>> attributes)
>>
>>/* If the precision of the type was specified with an attribute and it
>>   was too small, give an error.  Otherwise, use it.  */
>> -  if (TYPE_PRECISION (enumtype))
>> +  if (TYPE_PRECISION (enumtype) && lookup_attribute ("mode", attributes))
>>  {
>>if (precision > TYPE_PRECISION (enumtype))
>> {
>> @@ -8078,6 +8078,7 @@ finish_enum (tree enumtype, tree values, tree 
>> attributes)
>>TYPE_MIN_VALUE (enumtype) = TYPE_MIN_VALUE (tem);
>>TYPE_MAX_VALUE (enumtype) = TYPE_MAX_VALUE (tem);
>>TYPE_UNSIGNED (enumtype) = TYPE_UNSIGNED (tem);
>> +  TYPE_ALIGN (enumtype) = TYPE_ALIGN (tem);
>>TYPE_SIZE (enumtype) = 0;
>>TYPE_PRECISION (enumtype) = TYPE_PRECISION (tem);
>>
>> diff --git gcc/testsuite/gcc.dg/enum-incomplete-2.c 
>> gcc/testsuite/gcc.dg/enum-incomplete-2.c
>> new file mode 100644
>> index 000..5970551
>> --- /dev/null
>> +++ gcc/testsuite/gcc.dg/enum-incomplete-2.c
>> @@ -0,0 +1,41 @@
>> +/* PR c/52085 */
>> +/* { dg-do compile } */
>> +/* { dg-options "" } */
>> +
>> +#define SA(X) _Static_assert((X),#X)
>> +
>> +enum e1;
>> +enum e1 { A } __attribute__ ((__packed__));
>> +enum e2 { B } __attribute__ ((__packed__));
>> +SA (sizeof (enum e1) == sizeof (enum e2));
>> +SA (_Alignof (enum e1) == _Alignof (enum e2));
>> +
>> +enum e3;
>> +enum e3 { C = 256 } __attribute__ ((__packed__));
>> +enum e4 { D = 256 } __attribute__ ((__packed__));
>> +SA (sizeof (enum e3) == sizeof (enum e4));
>> +SA (_Alignof (enum e3) == _Alignof (enum e4));
>> +
>> +enum e5;
>> +enum e5 { E = __INT_MAX__ } __attribute__ ((__packed__));
>> +enum e6 { F = __INT_MAX__ } __attribute__ ((__packed__));
>> +SA (sizeof (enum e5) == sizeof (enum e6));
>> +SA (_Alignof (enum e5) == _Alignof (enum e6));
>> +
>> +enum e7;
>> +enum e7 { G } __attribute__ ((__mode__(__byte__)));
>> +enum e8 { H } __attribute__ ((__mode__(__byte__)));
>> +SA (sizeof (enum e7) == sizeof (enum e8));
>> +SA (_Alignof (enum e7) == _Alignof (enum e8));
>> +
>> +enum e9;
>> +enum e9 { I } __attribute__ ((__packed__, __mode__(__byte__)));
>> +enum e10 { J } __attribute__ ((__packed__, __mode__(__byte__)));
>> +SA (sizeof (enum e9) == sizeof (enum e10));
>> +SA (_Alignof (enum e9) == _Alignof (enum e10));
>> +
>> +enum e11;
>> +enum e11 { K } __attribute__ ((__mode__(__word__)));
>> +enum e12 { L } __attribute__ ((__mode__(__word__)));
>> +SA (sizeof (enum e11) == sizeof (enum e12));
>> +SA (_Alignof (enum e11) == _Alignof (enum e12));
>> diff --git gcc/testsuite/gcc.dg/enum-mode-1.c 
>> gcc/testsuite/gcc.dg/enum-mode-1.c
>> new file mode 100644
>> index 000..a701123
>> --- /dev/null
>> +++ gcc/testsuite/gcc.dg/enum-mode-1.c
>> @@ -0,0 +1,10 @@
>> +/* { dg-do co

[Patch] Backport fix for PR 52085 to gcc-5-branch?

[Senthil Kumar Selvaraj]

Hi,

  The fix for PR 52085 went into trunk when trunk was 6.0. I ran into the
  same issue on a gcc 5.x and found that the fix didn't get backported.

  Bootstrapped and reg tested below patch with x86-64-pc-linux. Ok to
  backport to gcc-5-branch?

Regards
Senthil

gcc/c/ChangeLog

2016-10-17  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

  Backport from mainline
2015-04-25  Marek Polacek  <pola...@redhat.com>
PR c/52085
* c-decl.c (finish_enum): Copy over TYPE_ALIGN.  Also check for "mode"
attribute.

gcc/testsuite/ChangeLog
2016-10-17  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

Backport from mainline
2015-04-25  Marek Polacek  <pola...@redhat.com>
PR c/52085
* gcc.dg/enum-incomplete-2.c: New test.
* gcc.dg/enum-mode-1.c: New test.


diff --git gcc/c/c-decl.c gcc/c/c-decl.c
index d1e7444..c508e7f 100644
--- gcc/c/c-decl.c
+++ gcc/c/c-decl.c
@@ -8050,7 +8050,7 @@ finish_enum (tree enumtype, tree values, tree attributes)
 
   /* If the precision of the type was specified with an attribute and it
  was too small, give an error.  Otherwise, use it.  */
-  if (TYPE_PRECISION (enumtype))
+  if (TYPE_PRECISION (enumtype) && lookup_attribute ("mode", attributes))
 {
   if (precision > TYPE_PRECISION (enumtype))
{
@@ -8078,6 +8078,7 @@ finish_enum (tree enumtype, tree values, tree attributes)
   TYPE_MIN_VALUE (enumtype) = TYPE_MIN_VALUE (tem);
   TYPE_MAX_VALUE (enumtype) = TYPE_MAX_VALUE (tem);
   TYPE_UNSIGNED (enumtype) = TYPE_UNSIGNED (tem);
+  TYPE_ALIGN (enumtype) = TYPE_ALIGN (tem);
   TYPE_SIZE (enumtype) = 0;
   TYPE_PRECISION (enumtype) = TYPE_PRECISION (tem);
 
diff --git gcc/testsuite/gcc.dg/enum-incomplete-2.c 
gcc/testsuite/gcc.dg/enum-incomplete-2.c
new file mode 100644
index 000..5970551
--- /dev/null
+++ gcc/testsuite/gcc.dg/enum-incomplete-2.c
@@ -0,0 +1,41 @@
+/* PR c/52085 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+#define SA(X) _Static_assert((X),#X)
+
+enum e1;
+enum e1 { A } __attribute__ ((__packed__));
+enum e2 { B } __attribute__ ((__packed__));
+SA (sizeof (enum e1) == sizeof (enum e2));
+SA (_Alignof (enum e1) == _Alignof (enum e2));
+
+enum e3;
+enum e3 { C = 256 } __attribute__ ((__packed__));
+enum e4 { D = 256 } __attribute__ ((__packed__));
+SA (sizeof (enum e3) == sizeof (enum e4));
+SA (_Alignof (enum e3) == _Alignof (enum e4));
+
+enum e5;
+enum e5 { E = __INT_MAX__ } __attribute__ ((__packed__));
+enum e6 { F = __INT_MAX__ } __attribute__ ((__packed__));
+SA (sizeof (enum e5) == sizeof (enum e6));
+SA (_Alignof (enum e5) == _Alignof (enum e6));
+
+enum e7;
+enum e7 { G } __attribute__ ((__mode__(__byte__)));
+enum e8 { H } __attribute__ ((__mode__(__byte__)));
+SA (sizeof (enum e7) == sizeof (enum e8));
+SA (_Alignof (enum e7) == _Alignof (enum e8));
+
+enum e9;
+enum e9 { I } __attribute__ ((__packed__, __mode__(__byte__)));
+enum e10 { J } __attribute__ ((__packed__, __mode__(__byte__)));
+SA (sizeof (enum e9) == sizeof (enum e10));
+SA (_Alignof (enum e9) == _Alignof (enum e10));
+
+enum e11;
+enum e11 { K } __attribute__ ((__mode__(__word__)));
+enum e12 { L } __attribute__ ((__mode__(__word__)));
+SA (sizeof (enum e11) == sizeof (enum e12));
+SA (_Alignof (enum e11) == _Alignof (enum e12));
diff --git gcc/testsuite/gcc.dg/enum-mode-1.c gcc/testsuite/gcc.dg/enum-mode-1.c
new file mode 100644
index 000..a701123
--- /dev/null
+++ gcc/testsuite/gcc.dg/enum-mode-1.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+
+enum e1 { A = 256 } __attribute__((__mode__(__byte__))); /* { dg-error 
"specified mode too small for enumeral values" } */
+enum e2 { B = 256 } __attribute__((__packed__, __mode__(__byte__))); /* { 
dg-error "specified mode too small for enumeral values" } */
+
+enum e3 { C = __INT_MAX__ } __attribute__((__mode__(__QI__))); /* { dg-error 
"specified mode too small for enumeral values" } */
+enum e4 { D = __INT_MAX__ } __attribute__((__packed__, __mode__(__QI__))); /* 
{ dg-error "specified mode too small for enumeral values" } */
+
+enum e5 { E = __INT_MAX__ } __attribute__((__mode__(__HI__))); /* { dg-error 
"specified mode too small for enumeral values" } */
+enum e6 { F = __INT_MAX__ } __attribute__((__packed__, __mode__(__HI__))); /* 
{ dg-error "specified mode too small for enumeral values" } */

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Bernd Schmidt writes:

> On 09/16/2016 09:02 PM, Senthil Kumar Selvaraj wrote:
>>   Does this make sense? I ran a reg test for the avr target with a
>>   slightly older version of this patch, it did not show any regressions.
>>   If this is the right fix, I'll make sure to run reg tests on x86_64
>>   after backporting to a gcc version where that target used reload.
>
> It's hard to say, and could have different effects on different targets.
> One thing though, at the very least the reg_class_size test would have 
> to be adapted - the idea is to find the largest class, and there's a 
> risk here of ending up with a large class that only has one valid register.

Agreed - I've updated the patch to compute rclass sizes based on regno
availability i.e., only if in_hard_reg_set_p and HARD_REGNO_MODE_OK, and
then use the computed sizes when calculating best_size.

>
> You'll also want to verify this against
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54814

Yes, this patch doesn't break the fix for PR54814. The change to
in_hard_reg_set_p was what fixed that, and that remains unmodified.

Reg tested this on top of trunk@190252 with the in_hard_reg_set_p
backport. x86_64-pc-linux bootstrapped and regtested ok. avr showed
no regressions either.

Ok for trunk?

Regards
Senthil

gcc/ChangeLog:

2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* reload.c (find_valid_class_1): Allow regclass if atleast one
regno in class is ok. Compute and use rclass size based on
actually available regnos for mode in rclass.

gcc/testsuite/ChangeLog:

2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.target/avr/pr71627.c: New.


Index: gcc/reload.c
===
--- gcc/reload.c(revision 240989)
+++ gcc/reload.c(working copy)
@@ -711,31 +711,36 @@
   enum reg_class best_class = NO_REGS;
   unsigned int best_size = 0;
   int cost;
+  unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 };
 
   for (rclass = 1; rclass < N_REG_CLASSES; rclass++)
 {
-  int bad = 0;
-  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++)
-   {
- if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
- && !HARD_REGNO_MODE_OK (regno, mode))
-   bad = 1;
-   }
-  
-  if (bad)
-   continue;
+  int atleast_one_regno_ok = 0;
 
+  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
+{
+  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
+{
+  atleast_one_regno_ok = 1;
+  if (HARD_REGNO_MODE_OK (regno, mode))
+computed_rclass_sizes[rclass]++;
+}
+}
+
+  if (!atleast_one_regno_ok)
+continue;
+
   cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
 
-  if ((reg_class_size[rclass] > best_size
-  && (best_cost < 0 || best_cost >= cost))
- || best_cost > cost)
-   {
- best_class = (enum reg_class) rclass;
- best_size = reg_class_size[rclass];
- best_cost = register_move_cost (outer, (enum reg_class) rclass,
- dest_class);
-   }
+  if ((computed_rclass_sizes[rclass] > best_size
+   && (best_cost < 0 || best_cost >= cost))
+  || best_cost > cost)
+{
+  best_class = (enum reg_class) rclass;
+  best_size = computed_rclass_sizes[rclass];
+  best_cost = register_move_cost (outer, (enum reg_class) rclass,
+  dest_class);
+}
 }
 
   gcc_assert (best_size != 0);

Index: gcc/testsuite/gcc.target/avr/pr71627.c
===
--- gcc/testsuite/gcc.target/avr/pr71627.c  (nonexistent)
+++ gcc/testsuite/gcc.target/avr/pr71627.c  (working copy)
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+
+extern volatile __memx const long  a, b, c, d, e, f;
+extern volatile long result;
+
+extern void vfunc (const char*, ...);
+
+void foo (void)
+{
+   result = a + b + c + d + e + f;
+   vfunc ("text", a, b, c, d, e, f, result);
+}

[Patch, testsuite] Fix gcc.g/tree-ssa/pr59597.c failure for avr

[Senthil Kumar Selvaraj]

Hi,

  This patch declares loop index variable j as a 32 bit int instead of
  assuming ints are 32 bits. The smaller int size on the avr makes prior
  passes optimize away the loop exit check (j < 1000), as the constant
  is outside the range of a 16 bit int.

  Committed to trunk, after reg testing with avr and x86_64-pc-linux

Regards
Senthil

gcc/testsuite/ChangeLog

2016-10-11  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/tree-ssa/pr59597.c: Typedef  __INT32_TYPE__ to i32.
(main): Declare j as i32 instead of int.

Index: gcc/testsuite/gcc.dg/tree-ssa/pr59597.c
===
--- gcc/testsuite/gcc.dg/tree-ssa/pr59597.c (revision 240974)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr59597.c (working copy)
@@ -4,6 +4,8 @@
 typedef unsigned short u16;
 typedef unsigned char u8;
 typedef unsigned int u32;
+__extension__ typedef __INT32_TYPE__ i32;
+
 long int random(int);
 #define NNN 10
 
@@ -37,7 +39,7 @@
 int
 main (int argc, char argv[])
 {
-  int i, j; u16 crc;
+  int i; i32 j; u16 crc;
   for (j = 0; j < 1000; j++)
 {
   for (i = 0; i < NNN; i++)

[Patch, testsuite] Fix pr69941.c test failure for avr

[Senthil Kumar Selvaraj]

Hi,

  The below patch fixes gcc.dg/torture/pr69941.c to pass for
  int size != 32 targets like avr.

  For the avr target, ints are 16 bits wide. VRP concludes
  that a right shift by 9 followed by an equality comparison 
  to 0x74 can never be true, and ends up eliminating the
  conditional. The code ends up unconditionally
  calling __builtin_abort and obviously fails when run.

  The patch fixes the testcase to use __INT32_TYPE__ (via a
  typedef) if __SIZEOF_INT__ is less than 4.

  Regtested with both avr and x86_64, the test passes with
  both targets.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-10-05  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/torture/pr69941.c: Use __INT32_TYPE__ instead
of int if __SIZEOF_INT__ is less than 4 bytes.

Index: gcc/testsuite/gcc.dg/torture/pr69941.c
===
--- gcc/testsuite/gcc.dg/torture/pr69941.c  (revision 240781)
+++ gcc/testsuite/gcc.dg/torture/pr69941.c  (working copy)
@@ -1,11 +1,17 @@
 /* { dg-do run } */
+
+#if __SIZEOF_INT__ < 4
+__extension__ typedef __INT32_TYPE__ int32_t;
+#else
+typedef int int32_t;
+#endif
  
 int a = 0;
 int b = 0;
 int c = 0;
-int e = 0;
+int32_t e = 0;
 int f = 0;
-int *g = 
+int32_t *g = 
  
 int fn1() { return b ? a : b; }
  

[Patch, testsuite] Add ffat-lto-objects to gcc.target/avr/torture/builtins_error.c

[Senthil Kumar Selvaraj]

Hi,

  This patch adds -ffat-lto-objects option to an avr target testcase.

  The compiler defaults to thin LTO objects if built with linker plugin
  support, and the error expected by the testcase appears only at link
  time, if at all. Forcing fat LTO object file creation generates the
  error consistently at compile, as expected.

  Committed to trunk.

Regards
Senthil

gcc/testsuite/ChangeLog:

2016-10-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.target/avr/torture/builtins-error.c: Add -ffat-lto-objects
  option.


Index: gcc/testsuite/gcc.target/avr/torture/builtins-error.c
===
--- gcc/testsuite/gcc.target/avr/torture/builtins-error.c   (revision 
240709)
+++ gcc/testsuite/gcc.target/avr/torture/builtins-error.c   (working copy)
@@ -1,4 +1,5 @@
 /* { dg-do assemble } */
+/* { dg-options "-ffat-lto-objects" } */
 
 char insert (long a)
 {

Re: [Patch, testsuite] Require int32plus for builtin-sprintf-warn-1.c

[Senthil Kumar Selvaraj]

James Greenhalgh writes:

> On Tue, Sep 27, 2016 at 04:40:22PM +0530, Senthil Kumar Selvaraj wrote:
>> Hi,
>> 
>>   This patch requires int32plus for
>>   gcc.dg/tree-ssa/builtin-sprintf-warn-1.c, as it reports a bunch of
>>   failures for a 16 bit int target like the avr. The "%u" format
>>   specifier tests, for example, use int literals big enough to only fit
>>   in a long int, and this causes unexpected warnings.
>> 
>>   Comitted to trunk.
>
> This change is obviously incomplete as it does not update the expected
> line numbers for warnings generated by this testcase.

Sorry for the breakage. While I tested that it reports UNSUPPORTED for
avr, I didn't test that it doesn't break other targets. I thought I'd
just modified behavior to skip the test, didn't realize the side effect
of adding a new line.

Guess Martin already has a patch fixing this and a couple of other
things (https://gcc.gnu.org/ml/gcc-patches/2016-09/msg02073.html).
Thanks Martin!

Regards
Senthil
>
> Found with my bisect robot:
>
>   Failures:
>   gcc.dg/tree-ssa/builtin-sprintf-warn-1.c 
>   
>   Bisected to: 
>
>   Author: saaadhu <saaadhu@138bc75d-0d04-0410-961f-82ee72b054a4>
>   Date:   Tue Sep 27 11:05:25 2016 +
>
> Fix bogus test failure for avr
> 
> The test has a bunch of hardcoded integer literals that would fit only in 
> a
> 32 bits+ int, causing overflow warnings for a 16 bit int target like avr.
> 
> gcc/testsuite/ChangeLog
> 
> 2016-09-27  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
> 
>   * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Require int32plus.
> 
> 
> git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@240528 
>
>
>   FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (nil) (test for warnings, 
> line 96)
> /* { dg-warning "nul past the end" "(nil)" { target *-linux-gnu 
> *-*-uclinux } 96 } */
>
>   FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c Glibc %p (test for warnings, 
> line 108)
> /* { dg-warning "nul past the end" "Glibc %p" { target *-linux-gnu } 108 
> } */
> /* { dg-warning "nul past the end" "Generic %p" { target *-*-uclinux } 
> 108 } */
>
>   FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
>
> The line numbers here need bumped to match the change you've made.
>
> Thanks,
> James
>
>
>> 2016-09-27  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>> 
>>  * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Require int32plus.
>>  
>>  PR fortran/77666
>> Index: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
>> ===
>> --- gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (revision 240524)
>> +++ gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (working copy)
>> @@ -1,5 +1,6 @@
>>  /* { dg-do compile } */
>>  /* { dg-options "-std=c99 -Wformat -Wformat-length=1 
>> -ftrack-macro-expansion=0" } */
>> +/* { dg-require-effective-target int32plus } */
>>  
>>  /* When debugging, define LINE to the line number of the test case to 
>> exercise
>> and avoid exercising any of the others.  The buffer and objsize macros
>> 

[Patch, testsuite] Require int32plus for builtin-sprintf-warn-1.c

[Senthil Kumar Selvaraj]

Hi,

  This patch requires int32plus for
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c, as it reports a bunch of
  failures for a 16 bit int target like the avr. The "%u" format
  specifier tests, for example, use int literals big enough to only fit
  in a long int, and this causes unexpected warnings.

  Comitted to trunk.

Regards
Senthil

2016-09-27  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Require int32plus.
 
PR fortran/77666
Index: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
===
--- gcc.dg/tree-ssa/builtin-sprintf-warn-1.c(revision 240524)
+++ gcc.dg/tree-ssa/builtin-sprintf-warn-1.c(working copy)
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-std=c99 -Wformat -Wformat-length=1 
-ftrack-macro-expansion=0" } */
+/* { dg-require-effective-target int32plus } */
 
 /* When debugging, define LINE to the line number of the test case to exercise
and avoid exercising any of the others.  The buffer and objsize macros

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Ping!

Regards
Senthil

Senthil Kumar Selvaraj writes:

> Hi,
>
>   The below patch fixes what I think are a couple of problems with
>   reload.c:find_valid_class_1.
>
>   First, even if no regno is in_hard_reg_set_p, it goes ahead and
>   considers rclass as valid. bad is set only if a regno is in the reg
>   class *and* HARD_REGNO_MODE_OK is false - if both are false, bad isn't
>   set and the reload gets a wrong rclass. If that happens to be the best
>   one, this eventually leads to find_reg running out of registers to
>   spill, because the chosen rclass won't have enough regs.
>
>   Second, it expects every regno in rclass to be valid for mode i.e., if
>   any regno fails HARD_REGNO_MODE_OK, it rejects the rclass. The
>   comments in the original commit for find_valid_class_1 say atleast one
>   regno is ok. This was updated to say "class which contains only
>   registers" when in_hard_reg_set_p was introduced in place of just
>   TEST_HARD_REG_BIT.
>
>   Is it meant to really test all regs? For the avr target, all regs are
>   8 bits wide, and HARD_REGNO_MODE_OK returns false for odd regnos if
>   mode != QImode. With the current code, it ignores a bunch of otherwise
>   perfectly legal reg classes.
>
>   To fix the first problem, the patch adds regno_in_rclass_mode to track
>   whether there's atleast one regno in hard_reg_set_p. To fix the second
>   problem, the patch sets bad initially, and resets it if
>   HARD_REGNO_MODE_OK succeeded for a regno, thus breaking the loop.
>
>   Of course, if both my points above are valid, we can do away with
>   regno_in_rclass_mode, just bad would do.
>
>   Does this make sense? I ran a reg test for the avr target with a
>   slightly older version of this patch, it did not show any regressions.
>   If this is the right fix, I'll make sure to run reg tests on x86_64
>   after backporting to a gcc version where that target used reload.
>
> Regards
> Senthil
>
>
> Index: reload.c
> ===
> --- reload.c  (revision 240185)
> +++ reload.c  (working copy)
> @@ -714,17 +714,22 @@
>  
>for (rclass = 1; rclass < N_REG_CLASSES; rclass++)
>  {
> -  int bad = 0;
> -  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++)
> - {
> -   if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
> -   && !HARD_REGNO_MODE_OK (regno, mode))
> - bad = 1;
> - }
> -  
> -  if (bad)
> - continue;
> +  int bad = 1;
> +  int regno_in_rclass_mode = 0;
>  
> +  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && bad; regno++)
> +{
> +  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
> +{
> +  regno_in_rclass_mode = 1;
> +  if (HARD_REGNO_MODE_OK (regno, mode))
> +bad = 0;
> +}
> +}
> +
> +  if (bad || !regno_in_rclass_mode)
> +continue;
> +
>cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
>  
>if ((reg_class_size[rclass] > best_size

Re: [Patch, avr] Backport fix for PR 65210 to gcc-5-branch

[Senthil Kumar Selvaraj]

Ping!

Regards
Senthil
Senthil Kumar Selvaraj writes:

> Hi,
>
>   Is it ok to backport PR 65210 to gcc-5-branch? The patch is already in
>   6.x and trunk.
>
> Regards
> Senthil
>
> gcc/ChangeLog
>
> 2016-09-22  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>
>   Backport from trunk r227496
>
>   PR target/65210
>   * config/avr/avr.c (avr_eval_addr_attrib): Look for io_low
>   attribute as well.
>
> gcc/testsuite/ChangeLog
>
> 2016-09-22  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>
>   Backport from trunk r227496
>
>   PR target/65210
>   * gcc.target/avr/pr65210.c: New test.
>
> Index: gcc/config/avr/avr.c
> ===
> --- gcc/config/avr/avr.c  (revision 240340)
> +++ gcc/config/avr/avr.c  (working copy)
> @@ -9122,6 +9122,8 @@
>if (SYMBOL_REF_FLAGS (x) & SYMBOL_FLAG_IO)
>   {
> attr = lookup_attribute ("io", DECL_ATTRIBUTES (decl));
> +   if (!attr || !TREE_VALUE (attr))
> + attr = lookup_attribute ("io_low", DECL_ATTRIBUTES (decl));
> gcc_assert (attr);
>   }
>if (!attr || !TREE_VALUE (attr))
>Index: gcc/testsuite/gcc.target/avr/pr65210.c
> ===
> --- gcc/testsuite/gcc.target/avr/pr65210.c(nonexistent)
> +++ gcc/testsuite/gcc.target/avr/pr65210.c(working copy)
> @@ -0,0 +1,7 @@
> +/* { dg-do compile } */
> +
> +/* This testcase exposes PR65210. Usage of the io_low attribute
> +   causes assertion failure because code only looks for the io
> +   attribute if SYMBOL_FLAG_IO is set. */
> +
> +volatile char q __attribute__((io_low,address(0x81)));

Backport fix for PR 65210 to gcc-5-branch

[Senthil Kumar Selvaraj]

Hi,

  Is it ok to backport PR 65210 to gcc-5-branch? The patch is already in
  6.x and trunk.

Regards
Senthil

gcc/ChangeLog

2016-09-22  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

Backport from trunk r227496

PR target/65210
* config/avr/avr.c (avr_eval_addr_attrib): Look for io_low
attribute as well.

gcc/testsuite/ChangeLog

2016-09-22  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

Backport from trunk r227496

PR target/65210
* gcc.target/avr/pr65210.c: New test.

Index: gcc/config/avr/avr.c
===
--- gcc/config/avr/avr.c(revision 240340)
+++ gcc/config/avr/avr.c(working copy)
@@ -9122,6 +9122,8 @@
   if (SYMBOL_REF_FLAGS (x) & SYMBOL_FLAG_IO)
{
  attr = lookup_attribute ("io", DECL_ATTRIBUTES (decl));
+ if (!attr || !TREE_VALUE (attr))
+   attr = lookup_attribute ("io_low", DECL_ATTRIBUTES (decl));
  gcc_assert (attr);
}
   if (!attr || !TREE_VALUE (attr))
   Index: gcc/testsuite/gcc.target/avr/pr65210.c
===
--- gcc/testsuite/gcc.target/avr/pr65210.c  (nonexistent)
+++ gcc/testsuite/gcc.target/avr/pr65210.c  (working copy)
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+
+/* This testcase exposes PR65210. Usage of the io_low attribute
+   causes assertion failure because code only looks for the io
+   attribute if SYMBOL_FLAG_IO is set. */
+
+volatile char q __attribute__((io_low,address(0x81)));

[Patch, avr] Provide correct LD offset bound in avr_address_cost

[Senthil Kumar Selvaraj]

Hi,

  This patch fixes cost computation in avr_address_cost - instead of the
  hardcoded 61, it uses the already existing MAX_LD_OFFSET(mode) macro.

  This showed up when investigating a code size regression in the ivopts
  pass. That pass computes address_cost with and without an offset to
  decide on the right induction variable candidate(s). The legitimate
  address target hook returns false for offsets more than 63, so the
  pass calls the TARGET_ADDRESS_COST hook with 62 as the offset.

  The hook implementation returns 18, and the ivopts pass concludes that
  the cost of address with *any* offset is 18, which is not true - the higher
  cost is incurred only with offsets bigger than MAX_LD_OFFSET. This in
  turn results in a suboptimal choice of induction variables in the
  ivopts pass. The patch changes the hardcoded 61 to use the mode
  specific MAX_LD_OFFSET instead.

  Regression testing with just that fix showed one additional
  compilation timeout. That turned out to be the same as
  https://lists.nongnu.org/archive/html/avr-gcc-list/2014-03/msg00010.html
  - the middle end takes too much time to decide on the best strategy to
  multiply DImode values on a 64 bit host. This already causes timeouts
  for a few builtin-arith-overflow-* tests (see
  https://gcc.gnu.org/ml/gcc-testresults/2016-09/msg02018.html), so it
  isn't really related to this fix. Just providing a cost estimate for
  DImode mul fixes the timeout though, so the patch does that by scaling
  SImode costs by 2 for DImode muls.

  With both changes in, there are no regressions, and the
  builtin-arith-overflow-* tests now PASS and don't timeout.

  Ok to commit to trunk and backport to 5.x?

Regards
Senthil
   

gcc/ChangeLog:

2016-09-21  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* config/avr/avr.c (avr_rtx_costs_1): Handle DImode MULT.
(avr_address_cost): Replace 61 with MAX_LD_OFFSET(mode).



diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index 148a61d..29f0022 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -10257,6 +10257,7 @@ avr_rtx_costs_1 (rtx x, machine_mode mode, int 
outer_code ATTRIBUTE_UNUSED,
   break;
 
case SImode:
+   case DImode:
  if (AVR_HAVE_MUL)
 {
   if (!speed)
@@ -10282,7 +10283,10 @@ avr_rtx_costs_1 (rtx x, machine_mode mode, int 
outer_code ATTRIBUTE_UNUSED,
 *total = COSTS_N_INSNS (AVR_HAVE_JMP_CALL ? 5 : 4);
 }
 
-  return true;
+  if (mode == DImode)
+*total *= 2;
+
+  return true;
 
default:
  return false;
@@ -10863,7 +10867,7 @@ avr_address_cost (rtx x, machine_mode mode 
ATTRIBUTE_UNUSED,
   && (REG_P (XEXP (x, 0))
   || GET_CODE (XEXP (x, 0)) == SUBREG))
 {
-  if (INTVAL (XEXP (x, 1)) >= 61)
+  if (INTVAL (XEXP (x, 1)) > MAX_LD_OFFSET(mode))
 cost = 18;
 }
   else if (CONSTANT_ADDRESS_P (x))

[Patch, testsuite] Make pr64130.c pass for avr

[Senthil Kumar Selvaraj]

Hi,
  
 For the lower vrp bound to be 2/-2, unsigned ints must be 4 bytes wide. This
 isn't true for targets like avr. Explicitly using __UINT32_TYPE__
 behind a typedef makes the testcase pass for all targets.

 Committed to trunk.

Regards
Senthil

gcc/testsuite/ChangeLog:

2016-09-21  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/tree-ssa/pr64130.c: Use __UINT32_TYPE__ instead of int.

 Index: gcc.dg/tree-ssa/pr64130.c
===
--- gcc.dg/tree-ssa/pr64130.c   (revision 240299)
+++ gcc.dg/tree-ssa/pr64130.c   (revision 240300)
@@ -2,12 +2,14 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fdump-tree-evrp" } */
 
-int funsigned (unsigned a)
+__extension__ typedef __UINT32_TYPE__ uint32_t;
+
+int funsigned (uint32_t a)
 {
   return 0x1L / a == 0;
 }
 
-int funsigned2 (unsigned a)
+int funsigned2 (uint32_t a)
 {
   if (a < 1) return 1;
   return (-1 * 0x1L) / a == 0;

[Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Hi,

  The below patch fixes what I think are a couple of problems with
  reload.c:find_valid_class_1.

  First, even if no regno is in_hard_reg_set_p, it goes ahead and
  considers rclass as valid. bad is set only if a regno is in the reg
  class *and* HARD_REGNO_MODE_OK is false - if both are false, bad isn't
  set and the reload gets a wrong rclass. If that happens to be the best
  one, this eventually leads to find_reg running out of registers to
  spill, because the chosen rclass won't have enough regs.

  Second, it expects every regno in rclass to be valid for mode i.e., if
  any regno fails HARD_REGNO_MODE_OK, it rejects the rclass. The
  comments in the original commit for find_valid_class_1 say atleast one
  regno is ok. This was updated to say "class which contains only
  registers" when in_hard_reg_set_p was introduced in place of just
  TEST_HARD_REG_BIT.

  Is it meant to really test all regs? For the avr target, all regs are
  8 bits wide, and HARD_REGNO_MODE_OK returns false for odd regnos if
  mode != QImode. With the current code, it ignores a bunch of otherwise
  perfectly legal reg classes.

  To fix the first problem, the patch adds regno_in_rclass_mode to track
  whether there's atleast one regno in hard_reg_set_p. To fix the second
  problem, the patch sets bad initially, and resets it if
  HARD_REGNO_MODE_OK succeeded for a regno, thus breaking the loop.

  Of course, if both my points above are valid, we can do away with
  regno_in_rclass_mode, just bad would do.

  Does this make sense? I ran a reg test for the avr target with a
  slightly older version of this patch, it did not show any regressions.
  If this is the right fix, I'll make sure to run reg tests on x86_64
  after backporting to a gcc version where that target used reload.

Regards
Senthil


Index: reload.c
===
--- reload.c(revision 240185)
+++ reload.c(working copy)
@@ -714,17 +714,22 @@
 
   for (rclass = 1; rclass < N_REG_CLASSES; rclass++)
 {
-  int bad = 0;
-  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++)
-   {
- if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
- && !HARD_REGNO_MODE_OK (regno, mode))
-   bad = 1;
-   }
-  
-  if (bad)
-   continue;
+  int bad = 1;
+  int regno_in_rclass_mode = 0;
 
+  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && bad; regno++)
+{
+  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
+{
+  regno_in_rclass_mode = 1;
+  if (HARD_REGNO_MODE_OK (regno, mode))
+bad = 0;
+}
+}
+
+  if (bad || !regno_in_rclass_mode)
+continue;
+
   cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
 
   if ((reg_class_size[rclass] > best_size

[Patch, testsuite] Require int32plus for pr70421.c

[Senthil Kumar Selvaraj]

Hi,

  This patch fixes a bogus testsuite failure for the avr target. The
  test has integer literals that only fit on targets with int size >= 32.

  Committed to trunk.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-09-16  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/torture/pr70421.c: Require int32plus.

Index: gcc.dg/torture/pr70421.c
===
--- gcc.dg/torture/pr70421.c(revision 240004)
+++ gcc.dg/torture/pr70421.c(working copy)
@@ -1,5 +1,6 @@
 /* PR target/70421 */
 /* { dg-do run } */
+/* { dg-require-effective-target int32plus } */
 /* { dg-additional-options "-Wno-psabi -w" } */
 
 typedef unsigned V __attribute__ ((vector_size (64)));

[Patch, testsuite, avr] Skip gcc.dg/Wno-frame-address.c for avr

[Senthil Kumar Selvaraj]

Hi,

  The below patch adds avr to the list of targets excluded for
  Wno-frame-address.c. Like the other excluded targets, the avr backend
  only supports the builtin for the current stack frame.

  Committed to trunk.

Regards
Senthil

gcc/testsuite/ChangeLog:

2016-09-06  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/Wno-frame-address.c: Skip for avr-*-*.


Index: gcc/testsuite/gcc.dg/Wno-frame-address.c
===
--- gcc/testsuite/gcc.dg/Wno-frame-address.c(revision 240004)
+++ gcc/testsuite/gcc.dg/Wno-frame-address.c(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-skip-if "Cannot access arbitrary stack frames" { arm*-*-* hppa*-*-* 
ia64-*-* visium-*-* } } */
+/* { dg-skip-if "Cannot access arbitrary stack frames" { arm*-*-* avr-*-* 
hppa*-*-* ia64-*-* visium-*-* } } */
 /* { dg-options "-Werror" } */
 /* { dg-additional-options "-mbackchain" { target { s390*-*-* } } } */
 

[Patch, testsuite] Fix more bogus failures for avr

[Senthil Kumar Selvaraj]

Hi,

  The below patch fixes some bogus testsuite failures for the avr
  target. The first three expect 32 bit ints, and the last one uses too
  much RAM for a typical avr device.

  Regtested with avr and x86_64-pc-linux, and committed to trunk.

Regards
Senthil


gcc/testsuite/ChangeLog

2016-09-01  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/pr64252.c: Require int32plus.
* gcc.dg/pr66299-1.c: Likewise.
* gcc.dg/pr66299-2.c: Likewise.
* gcc.dg/torture/20131115-1.c: Skip for avr.

Index: gcc/testsuite/gcc.dg/pr64252.c
===
--- gcc/testsuite/gcc.dg/pr64252.c  (revision 239920)
+++ gcc/testsuite/gcc.dg/pr64252.c  (working copy)
@@ -1,6 +1,7 @@
 /* PR target/64252 */
 /* { dg-do run } */
 /* { dg-options "-O2" } */
+/* { dg-require-effective-target int32plus } */
 
 typedef unsigned int V __attribute__((vector_size (32)));
 
Index: gcc/testsuite/gcc.dg/pr66299-1.c
===
--- gcc/testsuite/gcc.dg/pr66299-1.c(revision 239920)
+++ gcc/testsuite/gcc.dg/pr66299-1.c(working copy)
@@ -1,6 +1,7 @@
 /* PR tree-optimization/66299 */
 /* { dg-do run } */
 /* { dg-options "-fdump-tree-original" } */
+/* { dg-require-effective-target int32plus } */
 
 void
 test1 (int x)
Index: gcc/testsuite/gcc.dg/pr66299-2.c
===
--- gcc/testsuite/gcc.dg/pr66299-2.c(revision 239920)
+++ gcc/testsuite/gcc.dg/pr66299-2.c(working copy)
@@ -1,6 +1,7 @@
 /* PR tree-optimization/66299 */
 /* { dg-do run } */
 /* { dg-options "-fdump-tree-optimized -O" } */
+/* { dg-require-effective-target int32plus } */
 
 void
 test1 (int x, unsigned u)
Index: gcc/testsuite/gcc.dg/torture/20131115-1.c
===
--- gcc/testsuite/gcc.dg/torture/20131115-1.c   (revision 239920)
+++ gcc/testsuite/gcc.dg/torture/20131115-1.c   (working copy)
@@ -1,4 +1,5 @@
 /* { dg-do run } */
+/* { dg-skip-if "RAM usage too large" { "avr-*-*" } } */
 
 struct S { int i; };
 __attribute__((const, noinline, noclone))

[Patch, testsuite] Fix more bogus test failures for avr

[Senthil Kumar Selvaraj]

Hi,

  This patch fixes a couple of testsuite failures for the avr target by
  requiring int32plus support for zero_sign_ext_test.c, and explicitly
  using __UINT32_TYPE__ instead of assuming 32 bit ints for pr71083.c

  Regtested with avr and x86_64-pc-linux. Committed to trunk.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-08-24  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.c-torture/execute/pr71083.c: Use UINT32_TYPE instead
of unsigned int.
* gcc.dg/zero_sign_ext_test.c: Require int32plus.


Index: gcc/testsuite/gcc.c-torture/execute/pr71083.c
===
--- gcc/testsuite/gcc.c-torture/execute/pr71083.c   (revision 239731)
+++ gcc/testsuite/gcc.c-torture/execute/pr71083.c   (working copy)
@@ -1,5 +1,7 @@
+__extension__ typedef __UINT32_TYPE__ uint32_t;
+
 struct lock_chain {
-  unsigned int irq_context: 2,
+  uint32_t irq_context: 2,
 depth: 6,
 base: 24;
 };
Index: gcc/testsuite/gcc.dg/zero_sign_ext_test.c
===
--- gcc/testsuite/gcc.dg/zero_sign_ext_test.c   (revision 239731)
+++ gcc/testsuite/gcc.dg/zero_sign_ext_test.c   (working copy)
@@ -2,6 +2,7 @@
 
 /* { dg-options "-O2" } */
 /* { dg-do run } */
+/* { dg-require-effective-target int32plus } */
 
 #define TYPE_MAX(type, sign)   \
   ((!sign) ? ((1 << (sizeof (type) * 8 - 1)) - 1) :\
  

Re: [Patch, testsuite] Skip tests that expect 4 byte alignment for avr

[Senthil Kumar Selvaraj]

Jeff Law writes:

> On 08/11/2016 01:40 AM, Senthil Kumar Selvaraj wrote:
>> Hi,
>>
>>   The below patch adds the AVR target to the list of targets that don't
>>   have natural_alignment_32. It also skips ipa/propalign-*.c
>>   tests (which expect 4 byte alignment), if both
>>   natural_alignment_32 and natural_alignment_64 are false.
>>
>>   Is this the right way to fix this? Ok to commit?
>>
>> Regards
>> Senthil
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-08-11  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>
>>  * gcc.dg/ipa/propalign-1.c: Skip for targets with !natural_alignment_32
>>  and !natural_alignment_64.
>>  * gcc.dg/ipa/propalign-2.c: Likewise.
>>  * gcc.dg/ipa/propalign-3.c: Likewise.
>>  * gcc.dg/ipa/propalign-4.c: Likewise.
>>  * gcc.dg/ipa/propalign-5.c: Likewise.
>>  * lib/target-supports.exp
>>  (check_effective_target_natural_alignment_32): Add avr-*-*.
> Do you need to add an avr case to 
> check_effective_target_natural_alignment_64 as well?

The 64 bit version is written to return true only for lp64 and spu-*-*,
so I didn't have to do anything there.
>
> Have you tested this on anything other than avr?
>

Yes, x86_64-pc-linux passes these tests, like it did before my patch.

Regards
Senthil

[Patch, testsuite] Skip tests that expect 4 byte alignment for avr

[Senthil Kumar Selvaraj]

Hi,

  The below patch adds the AVR target to the list of targets that don't
  have natural_alignment_32. It also skips ipa/propalign-*.c
  tests (which expect 4 byte alignment), if both
  natural_alignment_32 and natural_alignment_64 are false.

  Is this the right way to fix this? Ok to commit?

Regards
Senthil

gcc/testsuite/ChangeLog:

2016-08-11  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/ipa/propalign-1.c: Skip for targets with !natural_alignment_32
and !natural_alignment_64.
* gcc.dg/ipa/propalign-2.c: Likewise.
* gcc.dg/ipa/propalign-3.c: Likewise.
* gcc.dg/ipa/propalign-4.c: Likewise.
* gcc.dg/ipa/propalign-5.c: Likewise.
* lib/target-supports.exp
(check_effective_target_natural_alignment_32): Add avr-*-*.

Index: gcc/testsuite/gcc.dg/ipa/propalign-1.c
===
--- gcc/testsuite/gcc.dg/ipa/propalign-1.c  (revision 239318)
+++ gcc/testsuite/gcc.dg/ipa/propalign-1.c  (working copy)
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-cp -fdump-tree-optimized" 
} */
+/* { dg-skip-if "No alignment restrictions" { { ! natural_alignment_32 } && { 
! natural_alignment_64 } } } */
 
 #include 
 
Index: gcc/testsuite/gcc.dg/ipa/propalign-2.c
===
--- gcc/testsuite/gcc.dg/ipa/propalign-2.c  (revision 239318)
+++ gcc/testsuite/gcc.dg/ipa/propalign-2.c  (working copy)
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fno-early-inlining -fdump-ipa-cp -fdump-tree-optimized" 
} */
+/* { dg-skip-if "No alignment restrictions" { { ! natural_alignment_32 } && { 
! natural_alignment_64 } } } */
 
 #include 
 
Index: gcc/testsuite/gcc.dg/ipa/propalign-3.c
===
--- gcc/testsuite/gcc.dg/ipa/propalign-3.c  (revision 239318)
+++ gcc/testsuite/gcc.dg/ipa/propalign-3.c  (working copy)
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fno-ipa-cp-alignment -fno-early-inlining -fdump-ipa-cp 
-fdump-tree-optimized" } */
+/* { dg-skip-if "No alignment restrictions" { { ! natural_alignment_32 } && { 
! natural_alignment_64 } } } */
 
 #include 
 
Index: gcc/testsuite/gcc.dg/ipa/propalign-4.c
===
--- gcc/testsuite/gcc.dg/ipa/propalign-4.c  (revision 239318)
+++ gcc/testsuite/gcc.dg/ipa/propalign-4.c  (working copy)
@@ -1,5 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fdump-ipa-cp"  } */
+/* { dg-skip-if "No alignment restrictions" { { ! natural_alignment_32 } && { 
! natural_alignment_64 } } } */
+
 int n;
 
 static void
Index: gcc/testsuite/gcc.dg/ipa/propalign-5.c
===
--- gcc/testsuite/gcc.dg/ipa/propalign-5.c  (revision 239318)
+++ gcc/testsuite/gcc.dg/ipa/propalign-5.c  (working copy)
@@ -1,5 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fdump-ipa-cp"  } */
+/* { dg-skip-if "No alignment restrictions" { { ! natural_alignment_32 } && { 
! natural_alignment_64 } } } */
+
 int n;
 
 static void
Index: gcc/testsuite/lib/target-supports.exp
===
--- gcc/testsuite/lib/target-supports.exp   (revision 239318)
+++ gcc/testsuite/lib/target-supports.exp   (working copy)
@@ -5221,7 +5221,8 @@
 } else {
 # FIXME: 32bit powerpc: guaranteed only if MASK_ALIGN_NATURAL/POWER.
 set et_natural_alignment_32_saved 1
-if { ([istarget *-*-darwin*] && [is-effective-target lp64]) } {
+if { ([istarget *-*-darwin*] && [is-effective-target lp64])
+ || [istarget avr-*-*] } {
 set et_natural_alignment_32_saved 0
 }
 }

Re: [Patch, tentative, reload] Make push_reload work with more types of subregs?

[Senthil Kumar Selvaraj]

Bernd Schmidt writes:

> On 08/08/2016 05:26 PM, Senthil Kumar Selvaraj wrote:
>
>> I picked out the commit where you'd added SYMBOL_REF handling (rev
>> #190252), and patched that with the below code. Bootstrapped and
>> regtested on x86_64-pc-linux - the results were identical with and
>> without the patch. Is this good enough for trunk?
>
>> -  else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
>> +  else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF
>> +   || CONSTANT_P (SUBREG_REG (in))
>> +   || GET_CODE (SUBREG_REG (in)) == PLUS)
>>  subreg_in_class = find_valid_class_1 (inmode,
>>GET_MODE (SUBREG_REG (in)),
>>rclass);
>
> Actually SYMBOL_REF should also be CONSTANT_P. For integers it looks 
> like it'll pass VOIDmode to find_valid_class_1 and just return NO_REGS. 
> which I suppose is OK.
>
> Would you mind removing the SYMBOL_REF test and retesting? Ok with that 
> change.

Bootstrapped and reg tested below patch with same setup as above - no
regressions showed up.

Committed patch to trunk. Ok for backport to 6.x and 5.x branches as
well?

Regards
Senthil

gcc/ChangeLog

2016-08-10  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

PR target/71873
* reload.c (push_reload): Compute subreg_in_class for
subregs of constants and plus expressions. Remove special
handling of SYMBOL_REFs.


gcc/testsuite/ChangeLog

2016-08-10  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

PR target/71873
* gcc.target/avr/pr71873.c: New test.



Index: gcc/reload.c
===
--- gcc/reload.c(revision 239318)
+++ gcc/reload.c(working copy)
@@ -1141,7 +1141,8 @@
   SUBREG_BYTE (in),
   GET_MODE (in)),
  REGNO (SUBREG_REG (in)));
-  else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
+  else if (CONSTANT_P (SUBREG_REG (in))
+   || GET_CODE (SUBREG_REG (in)) == PLUS)
subreg_in_class = find_valid_class_1 (inmode,
  GET_MODE (SUBREG_REG (in)),
  rclass);
Index: gcc/testsuite/gcc.target/avr/pr71873.c
===
--- gcc/testsuite/gcc.target/avr/pr71873.c  (nonexistent)
+++ gcc/testsuite/gcc.target/avr/pr71873.c  (working copy)
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fcaller-saves" } */
+
+#include 
+
+typedef struct {
+  uint8_t x;
+  uint32_t y;
+} A;
+
+A a;
+
+extern int bar(int);
+extern int foo (char *s, ...);
+
+extern uint8_t param;
+extern uint8_t h,m,s,ld,lm;
+extern uint16_t d;
+
+void gps_parse_string(int z)
+{
+  while (bar(z))
+  {
+switch (param)
+{
+  case 0: foo("a", , , , ); break;
+  case 1: foo("d", , , ); break;
+}
+  }
+}

[avr] fno-caller-saves and regression tests

[Senthil Kumar Selvaraj]

Hi Johann,

  Turning off -fcaller-saves for AVR makes the testcase I had for PR 71873
  pass unless I explicitly add -fcaller-saves to force the compiler to
  generate the triggering insn patterns.

  Wonder if we should modify the existing test cases in gcc.target/avr to
  be tested both ways (with and without caller saves)? At least the
  register allocation related ones probably won't catch regressions.

Regards
Senthil

Re: [Patch, tentative, reload] Make push_reload work with more types of subregs?

[Senthil Kumar Selvaraj]

Bernd Schmidt writes:

> On 07/28/2016 09:33 AM, Senthil Kumar Selvaraj wrote:
>>
>>   Is there a reason why only REG and SYMBOL_REFs get valid
>>   subreg_in_class? I tried extending it handle constants and PLUS
>>   expressions, and it fixes PR 71873. It also fixes a another
>>   bug that was a work around for the reload failure (PR 64452) - that
>>   had a plus expression instead of the const.
>>
>>   Reg testing on avr and x86_64 did not show any new failures. Is this
>>   the right way to fix this?
>
> I think it looks quite plausible. Note that testing x86_64 on trunk will 
> not do anything - it is no longer using reload. Could you go back to an 
> older branch (4.7 I think is the last one using reload) and retest 
> x86_64 with that, for better test coverage?

I picked out the commit where you'd added SYMBOL_REF handling (rev
#190252), and patched that with the below code. Bootstrapped and
regtested on x86_64-pc-linux - the results were identical with and 
without the patch. Is this good enough for trunk?

Regards
Senthil

gcc/ChangeLog:

2016-08-08  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

  PR reload/71873
* reload.c (push_reload): Compute subreg_in_class for
  subregs of constants and plus expressions.

gcc/testsuite/ChangeLog:

2016-08-08  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.target/avr/pr71873.c: New test.


Index: gcc/reload.c
===
--- gcc/reload.c(revision 239239)
+++ gcc/reload.c(working copy)
@@ -1141,7 +1141,9 @@
   SUBREG_BYTE (in),
   GET_MODE (in)),
  REGNO (SUBREG_REG (in)));
-  else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
+  else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF
+   || CONSTANT_P (SUBREG_REG (in))
+   || GET_CODE (SUBREG_REG (in)) == PLUS)
subreg_in_class = find_valid_class_1 (inmode,
  GET_MODE (SUBREG_REG (in)),
  rclass);
Index: gcc/testsuite/gcc.target/avr/pr71873.c
===
--- gcc/testsuite/gcc.target/avr/pr71873.c  (nonexistent)
+++ gcc/testsuite/gcc.target/avr/pr71873.c  (working copy)
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fcaller-saves" } */
+
+#include 
+
+typedef struct {
+  uint8_t x;
+  uint32_t y;
+} A;
+
+A a;
+
+extern int bar(int);
+extern int foo (char *s, ...);
+
+extern uint8_t param;
+extern uint8_t h,m,s,ld,lm;
+extern uint16_t d;
+
+void gps_parse_string(int z)
+{
+  while (bar(z))
+  {
+switch (param)
+{
+  case 0: foo("a", , , , ); break;
+  case 1: foo("d", , , ); break;
+}
+  }
+}

Re: [Patch, wwwdocs] Add caveat for AVR port

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> On 08.08.2016 10:24, Senthil Kumar Selvaraj wrote:
>> Hi,
>>
>>   This doc patch informs the user that a specific (or higher) version of
>>   binutils is a prerequisite for the fix for a rather vexing bug (PR
>>   71151) that was fixed for 6.2.
>>
>>   I've added it to the Caveats section; is there a better place? If not, ok 
>> to
>>   commit?
>>
>> Regards
>> Senthil
>>
>> Index: changes.html
>> ===
>> RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-6/changes.html,v
>> retrieving revision 1.86
>> diff -u -r1.86 changes.html
>> --- changes.html 20 Jun 2016 10:34:00 -  1.86
>> +++ changes.html 8 Aug 2016 08:17:13 -
>> @@ -40,6 +40,10 @@
>>  
>>  
>>
>> +The AVR port requires binutils version 2.26.1 or later for the 
>> fix
>> +for > href="https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71151;>PR71151
>> +to work.
>> +
>>
>>
>>  
>
>
> Maybe mention that also GCC 6.2 or higher is needed?  After all, it's a bug 
> in 
> the compiler and using respective Binutils without a fixed compiler is 
> pointless...

Ah, I thought the existing page would stay unmodified and my changes would
show up only for 6.2+ :(. I'll say

"With GCC 6.2 and later, the AVR port requires...".

Sounds good to you?

Regards
Senthil

[Patch, wwwdocs] Add caveat for AVR port

[Senthil Kumar Selvaraj]

Hi,

  This doc patch informs the user that a specific (or higher) version of
  binutils is a prerequisite for the fix for a rather vexing bug (PR
  71151) that was fixed for 6.2.

  I've added it to the Caveats section; is there a better place? If not, ok to
  commit?

Regards
Senthil

Index: changes.html
===
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-6/changes.html,v
retrieving revision 1.86
diff -u -r1.86 changes.html
--- changes.html20 Jun 2016 10:34:00 -  1.86
+++ changes.html8 Aug 2016 08:17:13 -
@@ -40,6 +40,10 @@
 
 
 
+The AVR port requires binutils version 2.26.1 or later for the fix
+for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71151;>PR71151
+to work.
+
   
 
 

[Patch, testsuite] Fix some more bogus failures for avr

[Senthil Kumar Selvaraj]

Hi,

Committed below patch to trunk as obvious.

Regards
Senthil


2016-08-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/init-excess-2.c: Require int32plus.
* gcc.dg/pr44024.c: Skip if target keeps null pointer checks.
* gcc.dg/pr59963-2.c: Require int32plus.
* gcc.dg/pr71084.c: Cast pointer to intprt_t.
* gcc.dg/unroll-7.c: Require int32plus.


Index: gcc.dg/init-excess-2.c
===
--- gcc.dg/init-excess-2.c  (revision 239064)
+++ gcc.dg/init-excess-2.c  (working copy)
@@ -3,6 +3,7 @@
c/71115 - Missing warning: excess elements in struct initializer.  */
 /* { dg-do compile } */
 /* { dg-options "" } */
+/* { dg-require-effective-target int32plus } */
 
 #include 
 
Index: gcc.dg/pr44024.c
===
--- gcc.dg/pr44024.c(revision 239064)
+++ gcc.dg/pr44024.c(working copy)
@@ -1,5 +1,6 @@
 /* { dg-do link } */
 /* { dg-options "-O1 -fdelete-null-pointer-checks -fdump-tree-ccp1" } */
+/* { dg-skip-if "" keeps_null_pointer_checks } */
 
 void foo();
 void link_error (void);
Index: gcc.dg/pr59963-2.c
===
--- gcc.dg/pr59963-2.c  (revision 239064)
+++ gcc.dg/pr59963-2.c  (working copy)
@@ -1,6 +1,7 @@
 /* PR c/59963 */
 /* { dg-do compile } */
 /* { dg-options "-Woverflow -Wconversion" } */
+/* { dg-require-effective-target int32plus } */
 
 extern void bar (unsigned char);
 extern void bar8 (unsigned char, unsigned char, unsigned char, unsigned char,
Index: gcc.dg/pr71084.c
===
--- gcc.dg/pr71084.c(revision 239064)
+++ gcc.dg/pr71084.c(working copy)
@@ -2,6 +2,8 @@
 /* { dg-do compile } */
 /* { dg-options "-O2" } */
 
+__extension__ typedef __INTPTR_TYPE__ intptr_t;
+
 void babl_format (void);
 void gimp_drawable_get_format (void);
 int _setjmp (void);
@@ -32,7 +34,7 @@
gimp_drawable_get_format();
   }
   for (; run_height;)
-for (; run_i < (long)fn1; ++run_i)
+for (; run_i < (long)(intptr_t)fn1; ++run_i)
   for (; width;)
 ;
 }
Index: gcc.dg/unroll-7.c
===
--- gcc.dg/unroll-7.c   (revision 239064)
+++ gcc.dg/unroll-7.c   (working copy)
@@ -1,5 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fdump-rtl-loop2_unroll -funroll-loops" } */
+/* { dg-require-effective-target int32plus } */
+
 int t(int *a)
 {
   int i;

Re: [patch,avr] PR70677: Use -fno-caller-saves for avr

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> On 02.08.2016 06:50, Senthil Kumar Selvaraj wrote:
>>
>> Denis Chertykov writes:
>>
>>> 2016-08-01 15:17 GMT+03:00 Georg-Johann Lay <a...@gjlay.de>:
>>>> Problem with -fcaller-saves is that there are situations where it triggers
>>>> an expensive frame just to store a variable around a function call even
>>>> though there are plenty of call-saved registers.
>>>>
>>>> Example:
>>>>
>>>> typedef __UINT8_TYPE__ uint8_t;
>>>>
>>>> extern uint8_t uart0_getc (void);
>>>>
>>>> void foo (uint8_t *buffer, uint8_t cnt)
>>>> {
>>>>   while (--cnt)
>>>> {
>>>>   *buffer++ = uart0_getc();
>>>> }
>>>> }
>>>>
>>>> $ avr-gcc -Os -S -dp -mmcu=atmega8 loop-buf.c
>>>>
>>>> $ avr-gcc gcc -B$TV -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c &&
>>>> avr-size loop-buf.o
>>>>textdata bss dec hex filename
>>>>  50   0   0  50  32 loop-buf.o
>>>>
>>>> $ avr-gcc -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c -fno-caller-saves
>>>> && avr-size loop-buf.o
>>>>textdata bss dec hex filename
>>>>  32   0   0  32  20 loop-buf.o
>>>>
>>>> I actually came never across a situation where -fcaller-saves improved the
>>>> code performance, hence this patch proposes to switch off -fcaller-saved 
>>>> per
>>>> default.
>>
>> Like you mentioned in the bug report, would fixing the costs be a better
>> way to fix this rather than a blanket disabling of the option?
>
> What costs specifically?  Where could the costs of different epilogues / 
> prologues be described?

I don't know either - I'd have to dig in to find that out too. Let me
check that out.

Regards
Senthil

Re: [patch,avr] PR70677: Use -fno-caller-saves for avr

[Senthil Kumar Selvaraj]

Denis Chertykov writes:

> 2016-08-01 15:17 GMT+03:00 Georg-Johann Lay :
>> Problem with -fcaller-saves is that there are situations where it triggers
>> an expensive frame just to store a variable around a function call even
>> though there are plenty of call-saved registers.
>>
>> Example:
>>
>> typedef __UINT8_TYPE__ uint8_t;
>>
>> extern uint8_t uart0_getc (void);
>>
>> void foo (uint8_t *buffer, uint8_t cnt)
>> {
>>   while (--cnt)
>> {
>>   *buffer++ = uart0_getc();
>> }
>> }
>>
>> $ avr-gcc -Os -S -dp -mmcu=atmega8 loop-buf.c
>>
>> $ avr-gcc gcc -B$TV -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c &&
>> avr-size loop-buf.o
>>textdata bss dec hex filename
>>  50   0   0  50  32 loop-buf.o
>>
>> $ avr-gcc -Os -c -save-temps -dp -mmcu=atmega8 loop-buf.c -fno-caller-saves
>> && avr-size loop-buf.o
>>textdata bss dec hex filename
>>  32   0   0  32  20 loop-buf.o
>>
>> I actually came never across a situation where -fcaller-saves improved the
>> code performance, hence this patch proposes to switch off -fcaller-saved per
>> default.

Like you mentioned in the bug report, would fixing the costs be a better
way to fix this rather than a blanket disabling of the option?

Regards
Senthil

[Patch, tentative, reload] Make push_reload work with more types of subregs?

[Senthil Kumar Selvaraj]

Hi,

  When analyzing PR 71873 (ICE in push_reload), I found that that code
  in push_reload that recursively calls push_reload for subreg
  expressions doesn't correctly set subreg_in_class for a few cases.

  Specifically, reload_inner_reg_of_subreg returns true if SUBREG_REG(x)
  is CONSTANT_P or if it's code is PLUS. The code that tries to find a
  valid class (before recursively calling push_reload), however, only
  does that if SUBREG_REG is REG_P or if it's a SYMBOL_REF. For the
  other cases, subreg_in_class is set to the default NO_REGS, and this
  triggers the rclass != NO_REGS assert just before find_reusable_reload.

  For PR 71873, reload sees

  (set (reg/f:HI 87)
(const:HI (plus:HI (symbol_ref:HI ("a")  )  
(const_int 1 [0x1] ../test.c:24 83 {*movhi}
 (expr_list:REG_EQUIV (const:HI (plus:HI (symbol_ref:HI ("a")  ) 
(const_int 1 [0x1])))
  and

  (set (mem:QI (post_dec:HI (reg/f:HI 32 __SP_L__)) [0  S1 A8]) 
(subreg:QI (reg/f:HI 87) 1))

  and decides to replace pseudo reg 87 in the latter insn with the
  REG_EQUIV it found in the former. The resulting RTL expression
   
  (subreg:QI (const:HI (plus:HI (symbol_ref:HI ("a") )
(const_int 1 [0x1]))) 1)

   does not match any of the conditions that handle subregs because
   subreg_low_part is false and the inner expr is not a REG or a SYMBOL_REF.

  Is there a reason why only REG and SYMBOL_REFs get valid
  subreg_in_class? I tried extending it handle constants and PLUS
  expressions, and it fixes PR 71873. It also fixes a another
  bug that was a work around for the reload failure (PR 64452) - that
  had a plus expression instead of the const.

  Reg testing on avr and x86_64 did not show any new failures. Is this
  the right way to fix this?

Regards
Senthil

diff --git gcc/reload.c gcc/reload.c
index 06426d9..f80d849 100644
--- gcc/reload.c
+++ gcc/reload.c
@@ -1141,7 +1141,9 @@ push_reload (rtx in, rtx out, rtx *inloc, rtx *outloc,
   SUBREG_BYTE (in),
   GET_MODE (in)),
  REGNO (SUBREG_REG (in)));
-  else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF)
+  else if (GET_CODE (SUBREG_REG (in)) == SYMBOL_REF
+   || CONSTANT_P (SUBREG_REG (in))
+   || GET_CODE (SUBREG_REG (in)) == PLUS)
subreg_in_class = find_valid_class_1 (inmode,
  GET_MODE (SUBREG_REG (in)),
  rclass);

Re: [Patch, testuite, committed] Fix some more tests that fail for non 32-bit int targets

[Senthil Kumar Selvaraj]

Mike Stump writes:

> On Jul 25, 2016, at 5:00 AM, Senthil Kumar Selvaraj 
> <senthil_kumar.selva...@atmel.com> wrote:
>> 
>>  The below patch fixes tests that fail for the avr target, because they
>>  assume ints are atleast 32 bits wide and pointers and longs have the
>>  same size.
>> 
>>  I've required int32plus support for one test, and for the other two,
>>  I've introduced a cast to intptr_t to avoid the pointer <-> int size
>>  difference warning.
>> 
>>  Reg tested on avr and x86_64 with no regressions. Committed as
>>  obvious.
>
> Can you use __INTPTR_TYPE__ instead?  This way, you don't need to add any 
> include, and the test case will work in cross environments without any libc, 
> which is at times handy.  Thanks.
>
> See grep INTPTR gcc/gcc/testsuite/gcc.dg/*.c for how people use it, and for 
> the unsigned version.
>
>> 2016-07-25  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>> 
>>  * gcc.dg/torture/pr69352.c (foo): Cast to intptr_t instead of long.
>>  * gcc.dg/torture/pr69771.c: Require int32plus.
>>  * gcc.dg/torture/pr71866.c (inb): Add cast to intptr_t.

I'll keep that in mind, thanks.

Is the below patch ok? I used uintptr_t in the second test as the source
type is unsigned long.

Regards
Senthil

Index: pr69352.c
===
--- pr69352.c   (revision 238743)
+++ pr69352.c   (working copy)
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
 
-#include 
+__extension__ typedef __INTPTR_TYPE__ intptr_t;
 
 int a[10][14], b, c, d, e, f, g, h, i;
 void bar (void);
Index: pr71866.c
===
--- pr71866.c   (revision 238743)
+++ pr71866.c   (working copy)
@@ -1,7 +1,8 @@
 /* { dg-do compile } */
 /* { dg-additional-options "-ftree-pre -fcode-hoisting" } */
 
-#include 
+__extension__ typedef __UINTPTR_TYPE__ uintptr_t;
+
 typedef unsigned char u8;
 extern unsigned long pci_io_base;
 u8 in_8 (const volatile void *);
@@ -26,7 +27,7 @@
 static inline
 u8 inb (unsigned long port)
 {
-  return readb((volatile void *)(intptr_t)pci_io_base + port);
+  return readb((volatile void *)(uintptr_t)pci_io_base + port);
 }
 static inline
 void outb (u8 val, unsigned long port)

[Patch, testuite, committed] Fix some more tests that fail for non 32-bit int targets

[Senthil Kumar Selvaraj]

Hi,

  The below patch fixes tests that fail for the avr target, because they
  assume ints are atleast 32 bits wide and pointers and longs have the
  same size.

  I've required int32plus support for one test, and for the other two,
  I've introduced a cast to intptr_t to avoid the pointer <-> int size
  difference warning.

  Reg tested on avr and x86_64 with no regressions. Committed as
  obvious.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-07-25  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/torture/pr69352.c (foo): Cast to intptr_t instead of long.
* gcc.dg/torture/pr69771.c: Require int32plus.
* gcc.dg/torture/pr71866.c (inb): Add cast to intptr_t.



Index: gcc.dg/torture/pr69352.c
===
--- gcc.dg/torture/pr69352.c(revision 238707)
+++ gcc.dg/torture/pr69352.c(working copy)
@@ -1,5 +1,7 @@
 /* { dg-do compile } */
 
+#include 
+
 int a[10][14], b, c, d, e, f, g, h, i;
 void bar (void);
 int
@@ -13,7 +15,7 @@
   else
 m = 13;
   if (a[x][m])
-l = (long) foo;
+l = (intptr_t) foo;
   a[x][i] = l;
   while (c)
 {
Index: gcc.dg/torture/pr69771.c
===
--- gcc.dg/torture/pr69771.c(revision 238707)
+++ gcc.dg/torture/pr69771.c(working copy)
@@ -1,5 +1,6 @@
 /* PR rtl-optimization/69771 */
 /* { dg-do compile } */
+/* { dg-require-effective-target int32plus } */
 
 unsigned char a = 5, c;
 unsigned short b = 0;
Index: gcc.dg/torture/pr71866.c
===
--- gcc.dg/torture/pr71866.c(revision 238707)
+++ gcc.dg/torture/pr71866.c(working copy)
@@ -1,6 +1,7 @@
 /* { dg-do compile } */
 /* { dg-additional-options "-ftree-pre -fcode-hoisting" } */
 
+#include 
 typedef unsigned char u8;
 extern unsigned long pci_io_base;
 u8 in_8 (const volatile void *);
@@ -25,7 +26,7 @@
 static inline
 u8 inb (unsigned long port)
 {
-  return readb((volatile void *)pci_io_base + port);
+  return readb((volatile void *)(intptr_t)pci_io_base + port);
 }
 static inline
 void outb (u8 val, unsigned long port)

Re: ubsan and Dejagnu not having a big enough buffer in some cases

[Senthil Kumar Selvaraj]

Richard Biener writes:

> On July 20, 2016 2:01:18 AM GMT+02:00, Andrew Pinski  
> wrote:
>>Hi,
>>  I noticed that ubsan testsuite sometimes has failures due to dejagnu
>>buffer gets full and we no longer match on the output any more.
>>As you can see from the .log file:
>>/data1/jenkins/workspace/BuildThunderX_native_gcc_6/gcc/gcc/testsuite/c-c++-common/ubsan/float-cast-overflow-1.c:88:3:
>>runtime error: value iPASS: c-c++-common/ubsan/float-cast-overflow-1.c
>>  -O0  execution test
>>
>>This looks like I am using a much bigger path name than what most
>>people use which is why I am seeing it fail.  Is there a way to
>>increase the buffer for dejagnu/expect so dg-output matches?  Looks
>>like it is limited to 500k (if I read dejagnu code correctly).
>>
>>Or can/should we split up float-cast-overflow-1.c instead?
>
> I see this for some of the larger C frontend tests with lots of expected 
> errors/warnings as well.

Are you guys getting this everytime or is it sporadic?

I recently ran into something similar, except that there were more parts
involved (Windows subsystem for Linux, wrapper process for gcc etc..)
and that it was random. After messing around with stdout/stderr buffer
sizes and finding they didn't really help, I eventually ran strace to
figure out what the heck was happening. The runtest process stops when a
read syscall gets EIO, but if it so happens that there's buffered data that's
already been read and not written yet, they get dropped.

9631  read(8, "\r\n/gcc/gcc/testsuite/gcc.dg/c99-vla-jump-1.c: 
I..e/gcc.dg/c99-vl", 4096) = 4096
9631  read(8, "a-jump-1.c:300:124: note...ote: 'f' declared here\r\r\n", 4096) 
= 1380
9631  write(7, "1.c:283:124: note: la..er with var", 4096) = 4096
9631  write(7, "iably modified typ..99-vla-jump-1.c:300:163: error: j", 1905) = 
1905
...
9631  write(4, "\0", 1) = 1
...
9631  read(8, 0x13a49b8, 4096)  = -1 EIO (Input/output error)
9631  fcntl(8, F_GETFL) = 0x802 (flags O_RDWR|O_NONBLOCK)
9631  fcntl(8, F_SETFL, O_RDWR) = 0
9631  fcntl(8, F_GETFL) = 0x2 (flags O_RDWR)
9631  close(8)  = 0
9631  open("/dev/null", O_RDONLY)   = 8
9631  fcntl(8, F_SETFD, FD_CLOEXEC) = 0
9631  fcntl(8, F_SETFD, FD_CLOEXEC) = 0
9631  wait4(9691, [{WIFEXITED(s) && WEXITSTATUS(s) == 1}], 0, NULL) = 9691
9631  close(8)  = 0
...
9631  write(7, "compiler exited with status 1\n", 30) = 30

The 1380 bytes read in the last read call end up getting dropped on the
floor. 

Andrew, is the 500K limit you see in remote.exp:standard_wait?

Regards
Senthil

[Patch, testsuite, tentative] Explicitly disable pointer <-> int cast warnings for avr?

[Senthil Kumar Selvaraj]

Hi,

  The patch fixes a couple of testsuite failures that show up for the
  avr target because it has different sizes for longs and pointers (4
  bytes versus 2), by explicitly disabling the warning for avr.

  Does this make sense? Skipping the test by requiring ptr32plus would
  have worked, but this lets the test run for avr and for compile tests
  that don't look at output, nothing should hopefully break. Is this a
  good strategy in general?

Regards
Senthil

diff --git gcc/testsuite/gcc.dg/torture/pr69352.c 
gcc/testsuite/gcc.dg/torture/pr69352.c
index ad718b9..cf06383 100644
--- gcc/testsuite/gcc.dg/torture/pr69352.c
+++ gcc/testsuite/gcc.dg/torture/pr69352.c
@@ -1,4 +1,5 @@
 /* { dg-do compile } */
+/* { dg-additional-options "-Wno-pointer-to-int-cast" { target { "avr-*-*" } } 
} */
 
 int a[10][14], b, c, d, e, f, g, h, i;
 void bar (void);
diff --git gcc/testsuite/gcc.dg/torture/pr71866.c 
gcc/testsuite/gcc.dg/torture/pr71866.c
index e1b36cb..c074ad4 100644
--- gcc/testsuite/gcc.dg/torture/pr71866.c
+++ gcc/testsuite/gcc.dg/torture/pr71866.c
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-additional-options "-ftree-pre -fcode-hoisting" } */
+/* { dg-additional-options "-Wno-int-to-pointer-cast" { target { "avr-*-*" } } 
} */
 
 typedef unsigned char u8;
 extern unsigned long pci_io_base;
-- 
2.7.4

[Patch, testsuite, committed] Fix gcc.dg/params/blocksort-part.c for non 32-bit int targets

[Senthil Kumar Selvaraj]

Hi,

  The below patch conditionally defines Int32 and UInt32 to accomodate
  targets with sizeof(int) != 4.

  Regtested with x86_64 and avr. Committed as obvious.

Regards
Senthil

2016-07-19  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/params/blocksort-part.c: Conditionally define Int32 
and UInt32 based on __SIZEOF_INT__.


--- gcc/testsuite/gcc.dg/params/blocksort-part.c
+++ gcc/testsuite/gcc.dg/params/blocksort-part.c
@@ -21,8 +21,13 @@
 typedef charChar;
 typedef unsigned char   Bool;
 typedef unsigned char   UChar;
+#if __SIZEOF_INT__ == 2
+typedef long Int32;
+typedef unsigned longUInt32;
+#else
 typedef int Int32;
 typedef unsigned intUInt32;
+#endif
 typedef short   Int16;
 typedef unsigned short  UInt16;
 
-- 
2.7.4

Re: [patch,avr] minor tweaks for 8-bit operations

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> This patch contains some unrelated tweaks
>
> - Supplying no-ldregs variant for andqi3, iorqi3 where a const_int mask 
> affects 
> only 1 bit
>
> - Some patterns that match situations with zero_extend that can be performed 
> with less instructions / register pressure.

>From my (admittedly limited) attempts at code size benchmarking, this
one will help a lot I think, considering ints are used everywhere and
they end up always taking 2 regs, even when one would do. Do you have any 
numbers on the code size improvement this provides?

Regards
Senthil
>
> - comparing HI against -1
>
> Ok for trunk?
>
> Johann
>
>
> gcc/
>   Minor tweaks for QImode.
>
>   * config/avr/predicates.md (const_m255_to_m1_operand): New.
>   * config/avr/constraints.md (Cn8, Ca1, Co1, Yx2): New constraints.
>   * config/avr/avr.md (add3) : Make "r,0,r" more
>   expensive.
>   (*cmphi.zero-extend.0, *cmphi.zero-extend.1)
>   (*usum_widenqihi3, *udiff_widenqihi3)
>   (*addhi3_zero_extend.const): New combiner insns.
>   (andqi3, iorqi3): Provide "l" (NO_LD_REGS) alternative if
>   just 1 bit is affected.
>   * config/avr/avr.c (avr_out_bitop) : Don't access xop[3].
>   (avr_out_compare) [EQ,NE]: Tweak comparing d-regs against -1.

[Patch, testsuite] Fix some bogus testsuite failures for avr

[Senthil Kumar Selvaraj]

Hi,

  This patch requires int32plus and ptr32plus for a couple of tests,
  tweaks Wduplicated-cond-3.c to use a smaller constant that fits in
  16 bits, and marks one test as too big
  for avr.

  Committed to trunk.

Regards
Senthil

2016-07-13  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* c-c++-common/Wduplicated-cond-3.c (fn10): Use smaller 
const literal.
* c-c++-common/builtin-arith-overflow-2.c: Skip for avr.
* c-c++-common/pr68833-1.c: Require int32plus.
* gcc.dg/ipa/pr63551.c: Likewise.
* gcc.dg/ipa/pr63595.c: Require ptr32plus.
* gcc.dg/ipa/pr64041.c: Require int32plus.
 
diff --git gcc/testsuite/c-c++-common/Wduplicated-cond-3.c 
gcc/testsuite/c-c++-common/Wduplicated-cond-3.c
index e3b5ac0..f928357 100644
--- gcc/testsuite/c-c++-common/Wduplicated-cond-3.c
+++ gcc/testsuite/c-c++-common/Wduplicated-cond-3.c
@@ -187,7 +187,7 @@ int
 fn10 (void)
 {
   if (foo ())
-return 1732984;
+return 17329;
   else if (foo ())
 return 18409;
   return 0;
diff --git gcc/testsuite/c-c++-common/builtin-arith-overflow-2.c 
gcc/testsuite/c-c++-common/builtin-arith-overflow-2.c
index 4cbceff..7dd0e50 100644
--- gcc/testsuite/c-c++-common/builtin-arith-overflow-2.c
+++ gcc/testsuite/c-c++-common/builtin-arith-overflow-2.c
@@ -1,6 +1,7 @@
 /* PR c/68120 - can't easily deal with integer overflow at compile time */
 /* { dg-do run } */
 /* { dg-additional-options "-Wno-long-long" } */
+/* { dg-skip-if "Program too big" { "avr-*-*" } } */
 
 #define SCHAR_MAX__SCHAR_MAX__
 #define SHRT_MAX __SHRT_MAX__
diff --git gcc/testsuite/c-c++-common/pr68833-1.c 
gcc/testsuite/c-c++-common/pr68833-1.c
index e0601b3..c88f67e 100644
--- gcc/testsuite/c-c++-common/pr68833-1.c
+++ gcc/testsuite/c-c++-common/pr68833-1.c
@@ -1,6 +1,7 @@
 /* PR c/68833 */
 /* { dg-do compile } */
 /* { dg-options "-Werror=larger-than-65536 -Werror=format 
-Werror=missing-noreturn" } */
+/* { dg-require-effective-target int32plus } */
 
 int a[131072]; /* { dg-error "size of 'a' is \[1-9]\[0-9]* bytes" } */
 int b[1024];   /* { dg-bogus "size of 'b' is \[1-9]\[0-9]* bytes" } */
diff --git gcc/testsuite/gcc.dg/ipa/pr63551.c gcc/testsuite/gcc.dg/ipa/pr63551.c
index 48b020a..225e323 100644
--- gcc/testsuite/gcc.dg/ipa/pr63551.c
+++ gcc/testsuite/gcc.dg/ipa/pr63551.c
@@ -1,5 +1,6 @@
 /* { dg-do run } */
 /* { dg-options "-Os" } */
+/* { dg-require-effective-target int32plus } */
 
 union U
 {
diff --git gcc/testsuite/gcc.dg/ipa/pr63595.c gcc/testsuite/gcc.dg/ipa/pr63595.c
index d656de5..ee48934 100644
--- gcc/testsuite/gcc.dg/ipa/pr63595.c
+++ gcc/testsuite/gcc.dg/ipa/pr63595.c
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fdump-ipa-icf-details"  } */
+/* { dg-require-effective-target ptr32plus } */
 
 typedef int size_t;
 
diff --git gcc/testsuite/gcc.dg/ipa/pr64041.c gcc/testsuite/gcc.dg/ipa/pr64041.c
index 4877b4b..18e0168 100644
--- gcc/testsuite/gcc.dg/ipa/pr64041.c
+++ gcc/testsuite/gcc.dg/ipa/pr64041.c
@@ -1,5 +1,6 @@
 /* { dg-do run } */
 /* { dg-options "-O2" } */
+/* { dg-require-effective-target int32plus } */
 
 int printf (const char *, ...);
 
-- 
2.7.4

Re: [Patch, avr] Fix PR 50739 - nameless error with -fmerge-all-constants

[Senthil Kumar Selvaraj]

Senthil Kumar Selvaraj writes:

> Hi,
>
>   This patch fixes a problem with fmerge-all-constants and the progmem
>   attribute - on trunk, the below testcase errors out with a section
>   conflict error.
>
>   When avr_asm_select_section renames .rodata.xyz section to
>   .progmem.xyz and calls get_section, it passes in the same flags in
>   sect. If the flags include SECTION_DECLARED, get_section barfs with a
>   section conflict error - the section flag comparison logic strips off
>   SECTION_DECLARED from existing section flags before comparing it with
>   the new incoming flags.
>
>   With -fmerge-all-constants, default_elf_select_section always returns
>   .rodata.strx.x. varasm switches to that section when writing out the
>   non progmem string literal, and that sets SECTION_DECLARED. The first
>   call to get_section with the section name transformed to
>   .progmem.data.strx.x then includes SECTION_DECLARED, but because this
>   is a new section, the section flag conflict logic doesn't kick in. The
>   second call to get_section, again including SECTION_DECLARED, triggers
>   the section flag conflict logic and causes the error.
>
>   Stripping off SECTION_DECLARED before calling get_section fixes the
>   problem - the flag is supposed to be set by switch_section anyway.
>
>   Reg testing showed no new regressions. Ok for trunk and backport to 6.x?
>
> Regards
> Senthil
>

Added missing description in Changelog entry.

gcc/testsuite/ChangeLog:

2016-07-05  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

    PR target/50739 
    * gcc.target/avr/pr50739.c: New test.


gcc/ChangeLog:

2016-07-05  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

PR target/50739 
* config/avr/avr.c (avr_asm_select_section): Strip off
SECTION_DECLARED from flags when calling get_section.

Regards
Senthil

>
> gcc/testsuite/ChangeLog:
>
> 2016-07-05  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>
>   PR target/50739 
>   * gcc.target/avr/pr50739.c: New test.
>
>
> gcc/ChangeLog:
>
> 2016-07-05  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>
>   PR target/50739 
>   * config/avr/avr.c (avr_asm_select_section):
>
>
> diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
> index 18ed766..9b7b392 100644
> --- gcc/config/avr/avr.c
> +++ gcc/config/avr/avr.c
> @@ -9641,7 +9641,7 @@ avr_asm_select_section (tree decl, int reloc, unsigned 
> HOST_WIDE_INT align)
>  {
>const char *sname = ACONCAT ((new_prefix,
>  name + strlen (old_prefix), 
> NULL));
> -  return get_section (sname, sect->common.flags, 
> sect->named.decl);
> +  return get_section (sname, sect->common.flags & 
> ~SECTION_DECLARED, sect->named.decl);
>  }
>  }
>  
> diff --git gcc/testsuite/gcc.target/avr/pr50739.c 
> gcc/testsuite/gcc.target/avr/pr50739.c
> new file mode 100644
> index 000..a6850b7
> --- /dev/null
> +++ gcc/testsuite/gcc.target/avr/pr50739.c
> @@ -0,0 +1,7 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fmerge-all-constants" } */
> +
> +char *ca = "123";
> +
> +const char a[] __attribute__((__progmem__))= "a";
> +const char b[] __attribute__((__progmem__))= "b";

[Patch, avr] Fix PR 50739 - nameless error with -fmerge-all-constants

[Senthil Kumar Selvaraj]

Hi,

  This patch fixes a problem with fmerge-all-constants and the progmem
  attribute - on trunk, the below testcase errors out with a section
  conflict error.

  When avr_asm_select_section renames .rodata.xyz section to
  .progmem.xyz and calls get_section, it passes in the same flags in
  sect. If the flags include SECTION_DECLARED, get_section barfs with a
  section conflict error - the section flag comparison logic strips off
  SECTION_DECLARED from existing section flags before comparing it with
  the new incoming flags.

  With -fmerge-all-constants, default_elf_select_section always returns
  .rodata.strx.x. varasm switches to that section when writing out the
  non progmem string literal, and that sets SECTION_DECLARED. The first
  call to get_section with the section name transformed to
  .progmem.data.strx.x then includes SECTION_DECLARED, but because this
  is a new section, the section flag conflict logic doesn't kick in. The
  second call to get_section, again including SECTION_DECLARED, triggers
  the section flag conflict logic and causes the error.

  Stripping off SECTION_DECLARED before calling get_section fixes the
  problem - the flag is supposed to be set by switch_section anyway.

  Reg testing showed no new regressions. Ok for trunk and backport to 6.x?

Regards
Senthil


gcc/testsuite/ChangeLog:

2016-07-05  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

PR target/50739 
* gcc.target/avr/pr50739.c: New test.


gcc/ChangeLog:

2016-07-05  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

PR target/50739 
* config/avr/avr.c (avr_asm_select_section):


diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index 18ed766..9b7b392 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -9641,7 +9641,7 @@ avr_asm_select_section (tree decl, int reloc, unsigned 
HOST_WIDE_INT align)
 {
   const char *sname = ACONCAT ((new_prefix,
 name + strlen (old_prefix), NULL));
-  return get_section (sname, sect->common.flags, sect->named.decl);
+  return get_section (sname, sect->common.flags & 
~SECTION_DECLARED, sect->named.decl);
 }
 }
 
diff --git gcc/testsuite/gcc.target/avr/pr50739.c 
gcc/testsuite/gcc.target/avr/pr50739.c
new file mode 100644
index 000..a6850b7
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr50739.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-fmerge-all-constants" } */
+
+char *ca = "123";
+
+const char a[] __attribute__((__progmem__))= "a";
+const char b[] __attribute__((__progmem__))= "b";
-- 
2.7.4

Re: How to improve the location of a gcc diagnostic

[Senthil Kumar Selvaraj]

David Malcolm writes:

> A user filed a bug about a bad location in a warning.  It was marked as
> an "easyhack" in bugzilla, and I had a go at fixing it.
>
> I though it may be useful for new GCC developers if I document what I
> did to fix it.
>
> FWIW, the bug was PR c/71610
>   i.e. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71610)
> ("Improve location for "warning: ISO C restricts enumerator values to
> range of 'int' [-Wpedantic]"?").
>
>
> Step 1: create a minimal reproducer for the diagnostic
>
> This was easy for this bug: I copied it from the bug report.  Try to
> avoid #include if possible; start with the simplest possible reproducer
> (to minimize effort in the debugger), and work from there.
>
> If you're working on a bug in our bugzilla it's good to click on the
> "take" button next to "Assignee" to mark yourself as assignee, and to
> set the status to ASSIGNED, so that we don't duplicate effort.
>
>
> Step 2: create a file under the relevant subdirectory of gcc/testsuite.
>
> This will be under one of gcc.dg, g++.dg or c-c++-common. If possible,
> create it under c-c++-common so that we can test both the C and C++
> frontends.
>
> I created a file called "pr71610.c" under c-c++-common.  (If this was
> for a specific warning, the name of the warning may have been better,
> but my case was part of "-Wpedantic", which is too broad for a testcase
> title).
>
>
> Step 3: run the reproducer under DejaGnu
>
> With a freshly built gcc, I ran the following from the "gcc" build
> directory:
>
>   make -jN -k && make check-gcc RUNTESTFLAGS="-v -v dg.exp=pr71610*"
>

Worth mentioning that you need to configure/make with CXXFLAGS or BOOT_*
flags set to -O0 -g3 so that breakpoints actually work?

Regards
Senthil

Re: [patch,avr]: ad PR71151: Make test cases pass on smaller targets.

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> On 22.06.2016 19:06, Mike Stump wrote:
>> On Jun 22, 2016, at 7:21 AM, Georg-Johann Lay wrote:
>>>
>>> Some tests for PR71151 assume that the target MCU has a 3-byte PC.  The
>>> tests are failing because the simulator (avrtest) rejects to load the
>>> respective executables if .text exceeds 128KiB, e.g. for -mmcu=atmega128
>>> which has only flash of 128KiB and only a 2-byte PC.
>>>
>>> Hence the tests have to be skipped if the target MCU has no 3-byte PC,
>>> hence a new dg-require-effective-target proc supporting "avr_3byte_pc".
>>>
>>> I added the new proc right after the last check_effective_target_arm_***
>>> so that the test is in ASCII collating order.
>>>
>>> Ok for trunk and v6?
>>
>> No.  Please see target-utils.exp and ensure that the tools generate a
>> stylized message and then add support for that to target-utils.exp.  If you
>> are using binutils, the text should go into a memory segment that will fill
>
> Binutils don't produce a message so there is nothing to scan for.  Hacking 
> binutils is beyond my scope.

binutils doesn't produce a message because

1. The size of text is not device specific right now - IIRC, it's set to
the max flash size for the emulation. I have a partial fix for this -
the text region size can now be set via a linker symbol
(__TEXT_REGION_LENGTH__). I'm planning to patch avr-libc to
automatically set this symbol based on flash size information
present in the device header file.

2. Even if (1) is fixed, the custom section (.foo) is not mapped to
any output section or region in the linker script. The linker can
error out only if the contents overflow a region.

If we have a custom linker script that places .foo in the text region,
and if we set the location counter to the address .foo should be placed,
i.e. something like


.text  :
{
 ...
 *(.fini0)  /* Infinite loop after program termination.  */
 KEEP (*(.fini0))

 . = FOO_START;
 KEEP(*(.foo))
 _etext = . ;
}  > text

and then if we pass -Wl,--defsym=FOO_START=0x20002 when linking, we'll get
the linker to report overflow.

Not sure if it's worth the effort though.

Mike, how about effective targets for sub arch/ISA variants
(avr51/avrxmega6/avrtiny..)? I guess arm has these for thumb1/thumb2/neon/dsp
etc. That would help us skip arch specific test cases, and will help
with testcases like these too - we can infer PC size from the arch.

Regards
Senthil

>
>> when it is too large.  When it does, then binutils will generate one of the
>> messages already handled, then you're done.
>
> avrtest behaves just as if the program under test would call abort.  There 
> are 
> at least 2 other AVR simulators; dunno how they would handle the situation.
>
> I don't see how an a-posteriori test could be independent of simulator, 
> independent of board descriptions and all that stuff.
>
> The tests in question don't fail because the program is too big as a result 
> of 
> some mussed optimization; some code is deliberately placed across a 64KiB or 
> 128KiB boundary or beyond 128KiB.  All this is known a priori.
>
> Hence dropping the original patch and proposing a new one that doesn't need 
> extensions to lib.
>
> The new tests just won't put any code at places where we know in advance some 
> simulator might barf.  As the compiler has no idea of exact flash size, the 
> relevant flash property is deduced from ISA properties.
>
> Is this one ok?
>
> Johann
>
>
> gcc/testsuite/
>   PR target/71151
>   * gcc.target/avr/pr71151-common.h (foo): Use macro SECTION_NAME
>   instead of ".foo" for its section name.
>   * gcc.target/avr/pr71151-2.c (SECTION_NAME): Define appropriately
>   depending on MCU's flash size.
>   * gcc.target/avr/pr71151-3.c (SECTION_NAME): Dito.
>   * gcc.target/avr/pr71151-4.c (SECTION_NAME): Dito.
>   * gcc.target/avr/pr71151-5.c (SECTION_NAME): Dito.
>   * gcc.target/avr/pr71151-6.c (SECTION_NAME): Dito.
>   * gcc.target/avr/pr71151-7.c (SECTION_NAME): Dito.
>   * gcc.target/avr/pr71151-8.c (SECTION_NAME): Dito.

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> Senthil Kumar Selvaraj schrieb:
>> Senthil Kumar Selvaraj writes:
>> 
>>> Georg-Johann Lay writes:
>>>
>>>> Senthil Kumar Selvaraj schrieb:
>>>>> Hi,
>>>>>
>>>>>   [set JUMP_TABLES_IN_TEXT_SECTION to 1]
>>>
>>> I added tests that use linker relaxation and discovered a relaxation bug
>>> in binutils 2.26 (and later) that messes up symbol values in the
>>> presence of alignment directives. I'm working on that right now -
>>> hopefully, it'll get backported to the release branch.
>>>
>>> Once that gets upstream, I'll resend the patch - with more tests, and
>>> incorporating your comments.
>>>
>> 
>> There were two binutils bugs (PR ld/20221 and ld/20254) that were
>> blocking this patch - on enabling, relaxation, jumptables were
>> getting corrupted. Both of the issues are now fixed, and the fixes
>> are in master and 2.26 branch.
>
> Should we mention in the release notes that Binutils >= 2.26 is needed 
> for avr-gcc >= 6 ?

Yes, we should document it. binutils 2.25 would probably work too, as
the bugs were introduced only in binutils 2.26. I'll check and send a patch.
>
> Maybe even check during configure whether an appropriate version of 
> Binutils is used?

That would be nice, but is it ok to add target specific conditions to
configure.ac?

Regards
Senthil

[Patch, testsuite] Mark some more tests as UNSUPPORTED for avr

[Senthil Kumar Selvaraj]

Hi,

 This patch fixes some bogus failures for the avr target by requiring
 int32plus or ptr32plus support.

 Ok for trunk?

Regards
Senthil


gcc/testsuite/ChangeLog:

2016-06-20  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* c-c++-common/pr68657-1.c: Require ptr32plus support.
* c-c++-common/pr68657-2.c: Likewise.
* c-c++-common/pr68657-3.c: Likewise.
* gcc.dg/torture/pr69714.c: Require int32plus support.
* gcc.dg/torture/pr70025.c: Likewise.
* gcc.dg/torture/pr70083.c: Likewise.
* gcc.dg/torture/pr70542.c: Likewise.
* gcc.dg/torture/pr70935.c: Require ptr32plus support.


diff --git gcc/testsuite/c-c++-common/pr68657-1.c 
gcc/testsuite/c-c++-common/pr68657-1.c
index 3db6f49..84f3e54 100644
--- gcc/testsuite/c-c++-common/pr68657-1.c
+++ gcc/testsuite/c-c++-common/pr68657-1.c
@@ -1,5 +1,6 @@
 /* PR c/68657 */
 /* { dg-options "-Werror=sign-conversion -Werror=float-conversion 
-Werror=frame-larger-than=65536" } */
+/* { dg-require-effective-target ptr32plus } */
 
 void
 f1 (void)
diff --git gcc/testsuite/c-c++-common/pr68657-2.c 
gcc/testsuite/c-c++-common/pr68657-2.c
index 9eb68ce..1391088 100644
--- gcc/testsuite/c-c++-common/pr68657-2.c
+++ gcc/testsuite/c-c++-common/pr68657-2.c
@@ -1,6 +1,7 @@
 /* PR c/68657 */
 /* { dg-do compile } */
 /* { dg-options "-Werror=larger-than=65536" } */
+/* { dg-require-effective-target ptr32plus } */
 
 int a[131072]; /* { dg-error "size of 'a' is \[1-9]\[0-9]* bytes" } */
 int b[1024];   /* { dg-bogus "size of 'b' is \[1-9]\[0-9]* bytes" } */
diff --git gcc/testsuite/c-c++-common/pr68657-3.c 
gcc/testsuite/c-c++-common/pr68657-3.c
index 84622fc..1e80c5b 100644
--- gcc/testsuite/c-c++-common/pr68657-3.c
+++ gcc/testsuite/c-c++-common/pr68657-3.c
@@ -1,5 +1,6 @@
 /* PR c/68657 */
 /* { dg-do compile } */
+/* { dg-require-effective-target ptr32plus } */
 
 #pragma GCC diagnostic error "-Wlarger-than=65536"
 int a[131072]; /* { dg-error "size of 'a' is \[1-9]\[0-9]* bytes" } */
diff --git gcc/testsuite/gcc.dg/torture/pr69714.c 
gcc/testsuite/gcc.dg/torture/pr69714.c
index 229b7ad..85de8be 100644
--- gcc/testsuite/gcc.dg/torture/pr69714.c
+++ gcc/testsuite/gcc.dg/torture/pr69714.c
@@ -1,5 +1,6 @@
 /* { dg-do run } */
 /* { dg-options "-fno-strict-aliasing" } */
+/* { dg-require-effective-target int32plus } */
 
 #include 
 #include 
diff --git gcc/testsuite/gcc.dg/torture/pr70025.c 
gcc/testsuite/gcc.dg/torture/pr70025.c
index dafae0b..6c43a0a 100644
--- gcc/testsuite/gcc.dg/torture/pr70025.c
+++ gcc/testsuite/gcc.dg/torture/pr70025.c
@@ -1,6 +1,7 @@
 /* PR middle-end/70025 */
 /* { dg-do run } */
 /* { dg-additional-options "-mtune=z10" { target s390*-*-* } } */
+/* { dg-require-effective-target int32plus } */
 
 typedef char (*F) (unsigned long, void *);
 typedef union { struct A { char a1, a2, a3, a4; unsigned long a5; F a6; void 
*a7; } b; char c[1]; } B;
diff --git gcc/testsuite/gcc.dg/torture/pr70083.c 
gcc/testsuite/gcc.dg/torture/pr70083.c
index 0cf2892..f33cb74 100644
--- gcc/testsuite/gcc.dg/torture/pr70083.c
+++ gcc/testsuite/gcc.dg/torture/pr70083.c
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-Wno-psabi" } */
+/* { dg-require-effective-target int32plus } */
 
 typedef short v16hi __attribute__ ((vector_size (32)));
 typedef int v8si __attribute__ ((vector_size (32)));
diff --git gcc/testsuite/gcc.dg/torture/pr70542.c 
gcc/testsuite/gcc.dg/torture/pr70542.c
index ed7ab9d..39c7f81 100644
--- gcc/testsuite/gcc.dg/torture/pr70542.c
+++ gcc/testsuite/gcc.dg/torture/pr70542.c
@@ -1,5 +1,6 @@
 /* PR rtl-optimization/70542 */
 /* { dg-do run } */
+/* { dg-require-effective-target int32plus } */
 
 int a[113], d[113];
 short b[113], c[113], e[113];
diff --git gcc/testsuite/gcc.dg/torture/pr70935.c 
gcc/testsuite/gcc.dg/torture/pr70935.c
index eb7f034..f1dd9e4 100644
--- gcc/testsuite/gcc.dg/torture/pr70935.c
+++ gcc/testsuite/gcc.dg/torture/pr70935.c
@@ -1,5 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O3 -g" } */
+/* { dg-require-effective-target ptr32plus } */
 
 int d0, sj, v0, rp, zi;
 
-- 
2.7.4

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> Senthil Kumar Selvaraj schrieb:
>> Hi,
>> 
>>   This patch fixes PR 71151 by eliminating the
>>   TARGET_ASM_FUNCTION_RODATA_SECTION hook and setting
>>   JUMP_TABLES_IN_TEXT_SECTION to 1.
>> 
>>   As described in the bugzilla entry, this hook assumed it will get
>>   called only for jumptable rodata for functions. This was true until
>>   6.1, when a commit in varasm.c started calling the hook for mergeable
>>   string/constant data as well.
>> 
>>   This resulted in string constants ending up in a section intended for
>>   jumptables (flash), and broke code using those constants, which
>>   expects them to be present in rodata (SRAM).
>> 
>>   Given that the original reason for placing jumptables in a section was
>>   fixed by Johann in PR 63323, this patch restores the original
>>   behavior. Reg testing on both gcc-6-branch and trunk showed no regressions.
>> 
>>   As pointed out by Johann, this may end up increasing code
>>   size if there are lots of branches that cross the jump tables. I
>>   intend to propose a separate patch that gives additional information
>>   to the target hook (SECCAT_RODATA_{STRING,JUMPTABLE}) so it can know
>>   what type of function rodata is coming on. Johann also suggested
>>   handling jump table generation ourselves - I'll experiment with that
>>   some more.
>> 
>>   If ok, could someone commit please? Could you also backport to
>>   gcc-6-branch?
>> 
>> Regards
>> Senthil
>> 
>> gcc/ChangeLog
>> 
>> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>> 
>
> Missing PR target/71151
>
>>  * config/avr/avr.c (avr_asm_function_rodata_section): Remove.
>>  * config/avr/avr.c (TARGET_ASM_FUNCTION_RODATA_SECTION): Remove.
>> 
>> gcc/testsuite/ChangeLog
>> 
>> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>> 
>
> Missing PR target/71151
>
>>  * gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
>>  * gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
>> 

Sorry I missed that. Is it ok to modify the entry alone again?

I just started using the mklog perl script from gcc/contrib
instead of writing the entries by hand - I didn't realize
that script doesn't know about PRs.

Regards
Senthil

MAINTAINERS (Write After Approval): Add myself

[Senthil Kumar Selvaraj]

Added myself to MAINTAINERS (Write After Approval).

Regards
Senthil

Index: ChangeLog
===
--- ChangeLog   (revision 237571)
+++ ChangeLog   (working copy)
@@ -1,3 +1,7 @@
+2016-06-18  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
+
+   * MAINTAINERS (Write After Approval): Add myself.
+
 2016-06-16  Michael Collison  <michael.colli...@arm.com>
 
* MAINTAINERS (Write After Approval): Add myself.
Index: MAINTAINERS
===
--- MAINTAINERS (revision 237571)
+++ MAINTAINERS (working copy)
@@ -569,6 +569,7 @@
 Martin Sebor   <mse...@gcc.gnu.org>
 Dodji Seketeli <do...@gcc.gnu.org>
 Svein Seldal   <sv...@dev.seldal.com>
+Senthil Kumar Selvaraj 
<senthil_kumar.selva...@atmel.com>
 Thiemo Seufer  <t...@networkno.de>
 Bill Seurer<seu...@linux.vnet.ibm.com>
 Marcus Shawcroft   <marcus.shawcr...@arm.com>

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

Denis Chertykov writes:

> 2016-06-16 10:27 GMT+03:00 Senthil Kumar Selvaraj
> <senthil_kumar.selva...@atmel.com>:
>>
>> Senthil Kumar Selvaraj writes:
>>
>>> Georg-Johann Lay writes:
>>>
>>>> Senthil Kumar Selvaraj schrieb:
>>>>> Hi,
>>>>>
>>>>>   This patch fixes PR 71151 by eliminating the
>>>>>   TARGET_ASM_FUNCTION_RODATA_SECTION hook and setting
>>>>>   JUMP_TABLES_IN_TEXT_SECTION to 1.
>>>>>
>>>>>   As described in the bugzilla entry, this hook assumed it will get
>>>>>   called only for jumptable rodata for functions. This was true until
>>>>>   6.1, when a commit in varasm.c started calling the hook for mergeable
>>>>>   string/constant data as well.
>>>>>
>>>>>   This resulted in string constants ending up in a section intended for
>>>>>   jumptables (flash), and broke code using those constants, which
>>>>>   expects them to be present in rodata (SRAM).
>>>>>
>>>>>   Given that the original reason for placing jumptables in a section was
>>>>>   fixed by Johann in PR 63323, this patch restores the original
>>>>>   behavior. Reg testing on both gcc-6-branch and trunk showed no 
>>>>> regressions.
>>>>
>>>> Just for the record:
>>>>
>>>> The intention for jump-tables in function-rodata-section was to get
>>>> fine-grained section for the tables so that --gc-sections and
>>>> -ffunction-sections not only gc unused functions but also unused
>>>> jump-tables.  As these tables had to reside in the lowest 64KiB of flash
>>>> (.progmem section) neither .rodata nor .text was a correct placement,
>>>> hence the hacking in TARGET_ASM_FUNCTION_RODATA_SECTION.
>>>>
>>>> Before using TARGET_ASM_FUNCTION_RODATA_SECTION, all jump tables were
>>>> put into .progmem.gcc_sw_table by ASM_OUTPUT_BEFORE_CASE_LABEL switching
>>>> to that section.
>>>>
>>>> We actually never had fump-tables in .text before...
>>>
>>> JUMP_TABLES_IN_TEXT_SECTION was 1 before r37465 - that was when the
>>> progmem.gcc_sw_table section was introduced. But yes, I understand that
>>> the target hook for FUNCTION_RODATA_SECTION was done to get them gc'ed
>>> along with the code.
>>>
>>>>
>>>> The purpose of PR63323 was to have more generic jump-table
>>>> implementation that also works if the table does NOT reside in the lower
>>>> 64KiB.  This happens when moving whole whole TEXT section around like
>>>> for a bootloader.
>>>
>>> Understood.
>>>>
>>>>>   As pointed out by Johann, this may end up increasing code
>>>>>   size if there are lots of branches that cross the jump tables. I
>>>>>   intend to propose a separate patch that gives additional information
>>>>>   to the target hook (SECCAT_RODATA_{STRING,JUMPTABLE}) so it can know
>>>>>   what type of function rodata is coming on. Johann also suggested
>>>>>   handling jump table generation ourselves - I'll experiment with that
>>>>>   some more.
>>>>>
>>>>>   If ok, could someone commit please? Could you also backport to
>>>>>   gcc-6-branch?
>>>>>
>>>>> Regards
>>>>> Senthil
>>>>>
>>>>> gcc/ChangeLog
>>>>>
>>>>> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>>>>
>>>>> * config/avr/avr.c (avr_asm_function_rodata_section): Remove.
>>>>> * config/avr/avr.c (TARGET_ASM_FUNCTION_RODATA_SECTION): Remove.
>>>>>
>>>>> gcc/testsuite/ChangeLog
>>>>>
>>>>> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>>>>
>>>>> * gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
>>>>> * gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
>>>>>
>>>>> diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
>>>>> index ba5cd91..3cb8cb7 100644
>>>>> --- gcc/config/avr/avr.c
>>>>> +++ gcc/config/avr/avr.c
>>>>> @@ -9488,65 +9488,6 @@ avr_asm_init_sections (void)
>>>>>  }
>>>>>
>>>>>
>>>>> -/* Implement `TARGET_ASM_FUNCTION_RODATA_SECTIO

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

Senthil Kumar Selvaraj writes:

> Georg-Johann Lay writes:
>
>> Senthil Kumar Selvaraj schrieb:
>>> Hi,
>>> 
>>>   This patch fixes PR 71151 by eliminating the
>>>   TARGET_ASM_FUNCTION_RODATA_SECTION hook and setting
>>>   JUMP_TABLES_IN_TEXT_SECTION to 1.
>>> 
>>>   As described in the bugzilla entry, this hook assumed it will get
>>>   called only for jumptable rodata for functions. This was true until
>>>   6.1, when a commit in varasm.c started calling the hook for mergeable
>>>   string/constant data as well.
>>> 
>>>   This resulted in string constants ending up in a section intended for
>>>   jumptables (flash), and broke code using those constants, which
>>>   expects them to be present in rodata (SRAM).
>>> 
>>>   Given that the original reason for placing jumptables in a section was
>>>   fixed by Johann in PR 63323, this patch restores the original
>>>   behavior. Reg testing on both gcc-6-branch and trunk showed no 
>>> regressions.
>>
>> Just for the record:
>>
>> The intention for jump-tables in function-rodata-section was to get 
>> fine-grained section for the tables so that --gc-sections and 
>> -ffunction-sections not only gc unused functions but also unused 
>> jump-tables.  As these tables had to reside in the lowest 64KiB of flash 
>> (.progmem section) neither .rodata nor .text was a correct placement, 
>> hence the hacking in TARGET_ASM_FUNCTION_RODATA_SECTION.
>>
>> Before using TARGET_ASM_FUNCTION_RODATA_SECTION, all jump tables were 
>> put into .progmem.gcc_sw_table by ASM_OUTPUT_BEFORE_CASE_LABEL switching 
>> to that section.
>>
>> We actually never had fump-tables in .text before...
>
> JUMP_TABLES_IN_TEXT_SECTION was 1 before r37465 - that was when the
> progmem.gcc_sw_table section was introduced. But yes, I understand that
> the target hook for FUNCTION_RODATA_SECTION was done to get them gc'ed
> along with the code.
>
>>
>> The purpose of PR63323 was to have more generic jump-table 
>> implementation that also works if the table does NOT reside in the lower 
>> 64KiB.  This happens when moving whole whole TEXT section around like 
>> for a bootloader.
>
> Understood.
>>
>>>   As pointed out by Johann, this may end up increasing code
>>>   size if there are lots of branches that cross the jump tables. I
>>>   intend to propose a separate patch that gives additional information
>>>   to the target hook (SECCAT_RODATA_{STRING,JUMPTABLE}) so it can know
>>>   what type of function rodata is coming on. Johann also suggested
>>>   handling jump table generation ourselves - I'll experiment with that
>>>   some more.
>>> 
>>>   If ok, could someone commit please? Could you also backport to
>>>   gcc-6-branch?
>>> 
>>> Regards
>>> Senthil
>>> 
>>> gcc/ChangeLog
>>> 
>>> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>> 
>>> * config/avr/avr.c (avr_asm_function_rodata_section): Remove.
>>> * config/avr/avr.c (TARGET_ASM_FUNCTION_RODATA_SECTION): Remove.
>>> 
>>> gcc/testsuite/ChangeLog
>>> 
>>> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>>> 
>>> * gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
>>> * gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
>>> 
>>> diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
>>> index ba5cd91..3cb8cb7 100644
>>> --- gcc/config/avr/avr.c
>>> +++ gcc/config/avr/avr.c
>>> @@ -9488,65 +9488,6 @@ avr_asm_init_sections (void)
>>>  }
>>>  
>>>  
>>> -/* Implement `TARGET_ASM_FUNCTION_RODATA_SECTION'.  */
>>> -
>>> -static section*
>>> -avr_asm_function_rodata_section (tree decl)
>>> -{
>>> -  /* If a function is unused and optimized out by -ffunction-sections
>>> - and --gc-sections, ensure that the same will happen for its jump
>>> - tables by putting them into individual sections.  */
>>> -
>>> -  unsigned int flags;
>>> -  section * frodata;
>>> -
>>> -  /* Get the frodata section from the default function in varasm.c
>>> - but treat function-associated data-like jump tables as code
>>> - rather than as user defined data.  AVR has no constant pools.  */
>>> -  {
>>> -int fdata = flag_data_sections;
>>> -
>>> -fla

[Patch, testsuite] Skip some more tests for targets with int size < 32

[Senthil Kumar Selvaraj]

Hi,

  This patch requires int32plus support for a few more tests - these
  were failing for the avr target.

  bswap-2.c uses left shifts wider than 16 bits on a char, and
  pr68067-{1,2} use an out of range negative number (INT_MIN for 32 bit int).

  If this is ok, could someone commit please? I don't have commit access.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-06-08  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.c-torture/execute/bswap-2.c: Require int32plus.
*   gcc.dg/torture/pr68067-1.c: Likewise.
* gcc.dg/torture/pr68067-2.c: Likewise.

diff --git gcc/testsuite/gcc.c-torture/execute/bswap-2.c 
gcc/testsuite/gcc.c-torture/execute/bswap-2.c
index 88132fe..63e7807 100644
--- gcc/testsuite/gcc.c-torture/execute/bswap-2.c
+++ gcc/testsuite/gcc.c-torture/execute/bswap-2.c
@@ -1,3 +1,5 @@
+/* { dg-require-effective-target int32plus } */
+
 #ifdef __UINT32_TYPE__
 typedef __UINT32_TYPE__ uint32_t;
 #else
diff --git gcc/testsuite/gcc.dg/torture/pr68067-1.c 
gcc/testsuite/gcc.dg/torture/pr68067-1.c
index a7b6aa0..f8ad3ca 100644
--- gcc/testsuite/gcc.dg/torture/pr68067-1.c
+++ gcc/testsuite/gcc.dg/torture/pr68067-1.c
@@ -1,4 +1,5 @@
 /* { dg-do run } */
+/* { dg-require-effective-target int32plus } */
 
 int main()
 {
diff --git gcc/testsuite/gcc.dg/torture/pr68067-2.c 
gcc/testsuite/gcc.dg/torture/pr68067-2.c
index 38a459b..e03bf22 100644
--- gcc/testsuite/gcc.dg/torture/pr68067-2.c
+++ gcc/testsuite/gcc.dg/torture/pr68067-2.c
@@ -1,4 +1,5 @@
 /* { dg-do run } */
+/* { dg-require-effective-target int32plus } */
 
 int main()
 {
-- 
2.7.4

[Patch, avr] Fix broken stack-usage-1.c test

[Senthil Kumar Selvaraj]

Hi,

  A recent patch I submitted fixed broken -fstack-usage for the avr
  target, by including the size of the return address pushed to the stack
  (https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01715.html).

  I forgot to send this testcase modification with that patch - here's
  the fix for making gcc.dg/stack-usage-1.c pass again for avr.

  If this is ok, could someone commit please? I don't have commit
  access.

Regards
Senthil

gcc/testsuite/ChangeLog

2016-06-08  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>

* gcc.dg/stack-usage-1.c (SIZE): Consider return address
  when setting SIZE.

diff --git gcc/testsuite/gcc.dg/stack-usage-1.c 
gcc/testsuite/gcc.dg/stack-usage-1.c
index 7864c6a..bdc5656 100644
--- gcc/testsuite/gcc.dg/stack-usage-1.c
+++ gcc/testsuite/gcc.dg/stack-usage-1.c
@@ -64,7 +64,11 @@
 #define SIZE 240
 #  endif
 #elif defined (__AVR__)
-#  define SIZE 254
+#if defined (__AVR_3_BYTE_PC__ )
+#  define SIZE 251 /* 256 - 2 bytes for Y - 3 bytes for return address */
+#else
+#  define SIZE 252 /* 256 - 2 bytes for Y - 2 bytes for return address */
+#endif
 #elif defined (__s390x__)
 #  define SIZE 96  /* 256 - 160 bytes for register save area */
 #elif defined (__s390__)

Re: [Patch, avr] Fix PR 71151

[Senthil Kumar Selvaraj]

Georg-Johann Lay writes:

> Senthil Kumar Selvaraj schrieb:
>> Hi,
>> 
>>   This patch fixes PR 71151 by eliminating the
>>   TARGET_ASM_FUNCTION_RODATA_SECTION hook and setting
>>   JUMP_TABLES_IN_TEXT_SECTION to 1.
>> 
>>   As described in the bugzilla entry, this hook assumed it will get
>>   called only for jumptable rodata for functions. This was true until
>>   6.1, when a commit in varasm.c started calling the hook for mergeable
>>   string/constant data as well.
>> 
>>   This resulted in string constants ending up in a section intended for
>>   jumptables (flash), and broke code using those constants, which
>>   expects them to be present in rodata (SRAM).
>> 
>>   Given that the original reason for placing jumptables in a section was
>>   fixed by Johann in PR 63323, this patch restores the original
>>   behavior. Reg testing on both gcc-6-branch and trunk showed no regressions.
>
> Just for the record:
>
> The intention for jump-tables in function-rodata-section was to get 
> fine-grained section for the tables so that --gc-sections and 
> -ffunction-sections not only gc unused functions but also unused 
> jump-tables.  As these tables had to reside in the lowest 64KiB of flash 
> (.progmem section) neither .rodata nor .text was a correct placement, 
> hence the hacking in TARGET_ASM_FUNCTION_RODATA_SECTION.
>
> Before using TARGET_ASM_FUNCTION_RODATA_SECTION, all jump tables were 
> put into .progmem.gcc_sw_table by ASM_OUTPUT_BEFORE_CASE_LABEL switching 
> to that section.
>
> We actually never had fump-tables in .text before...

JUMP_TABLES_IN_TEXT_SECTION was 1 before r37465 - that was when the
progmem.gcc_sw_table section was introduced. But yes, I understand that
the target hook for FUNCTION_RODATA_SECTION was done to get them gc'ed
along with the code.

>
> The purpose of PR63323 was to have more generic jump-table 
> implementation that also works if the table does NOT reside in the lower 
> 64KiB.  This happens when moving whole whole TEXT section around like 
> for a bootloader.

Understood.
>
>>   As pointed out by Johann, this may end up increasing code
>>   size if there are lots of branches that cross the jump tables. I
>>   intend to propose a separate patch that gives additional information
>>   to the target hook (SECCAT_RODATA_{STRING,JUMPTABLE}) so it can know
>>   what type of function rodata is coming on. Johann also suggested
>>   handling jump table generation ourselves - I'll experiment with that
>>   some more.
>> 
>>   If ok, could someone commit please? Could you also backport to
>>   gcc-6-branch?
>> 
>> Regards
>> Senthil
>> 
>> gcc/ChangeLog
>> 
>> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>> 
>>  * config/avr/avr.c (avr_asm_function_rodata_section): Remove.
>>  * config/avr/avr.c (TARGET_ASM_FUNCTION_RODATA_SECTION): Remove.
>> 
>> gcc/testsuite/ChangeLog
>> 
>> 2016-06-03  Senthil Kumar Selvaraj  <senthil_kumar.selva...@atmel.com>
>> 
>>  * gcc/testsuite/gcc.target/avr/pr71151-1.c: New.
>>  * gcc/testsuite/gcc.target/avr/pr71151-2.c: New.
>> 
>> diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
>> index ba5cd91..3cb8cb7 100644
>> --- gcc/config/avr/avr.c
>> +++ gcc/config/avr/avr.c
>> @@ -9488,65 +9488,6 @@ avr_asm_init_sections (void)
>>  }
>>  
>>  
>> -/* Implement `TARGET_ASM_FUNCTION_RODATA_SECTION'.  */
>> -
>> -static section*
>> -avr_asm_function_rodata_section (tree decl)
>> -{
>> -  /* If a function is unused and optimized out by -ffunction-sections
>> - and --gc-sections, ensure that the same will happen for its jump
>> - tables by putting them into individual sections.  */
>> -
>> -  unsigned int flags;
>> -  section * frodata;
>> -
>> -  /* Get the frodata section from the default function in varasm.c
>> - but treat function-associated data-like jump tables as code
>> - rather than as user defined data.  AVR has no constant pools.  */
>> -  {
>> -int fdata = flag_data_sections;
>> -
>> -flag_data_sections = flag_function_sections;
>> -frodata = default_function_rodata_section (decl);
>> -flag_data_sections = fdata;
>> -flags = frodata->common.flags;
>> -  }
>> -
>> -  if (frodata != readonly_data_section
>> -  && flags & SECTION_NAMED)
>> -{
>> -  /* Adjust section flags and replace section name prefix.  */
>> -
>> -  unsigned int i;
>> -
>> -  st