* One more path to fix PR70478
@ 2017-04-10 15:05 Vladimir Makarov
2017-04-11 7:30 ` Christophe Lyon
0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Makarov @ 2017-04-10 15:05 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 304 bytes --]
This is the second try to fix
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70478
The first try patch triggered a latent bug and broke one Fortran
testcase on x86-64.
The patch was successfully bootstrapped on x86-64 and tested on
x86-64, ppc64, and aarch64.
Committed as rev. 246808.
[-- Attachment #2: pr70478-2.patch --]
[-- Type: text/x-patch, Size: 3908 bytes --]
Index: ChangeLog
===================================================================
--- ChangeLog (revision 246807)
+++ ChangeLog (working copy)
@@ -1,3 +1,12 @@
+2017-04-10 Vladimir Makarov <vmakarov@redhat.com>
+
+ PR rtl-optimization/70478
+ * lra-constraints.c (curr_small_class_check): New.
+ (update_and_check_small_class_inputs): New.
+ (process_alt_operands): Update curr_small_class_check. Disfavor
+ alternative insn memory operands. Check available regs for small
+ class operands.
+
2017-03-31 Matthew Fortune <matthew.fortune@imgtec.com>
PR target/80057
Index: lra-constraints.c
===================================================================
--- lra-constraints.c (revision 246789)
+++ lra-constraints.c (working copy)
@@ -1852,6 +1852,42 @@ prohibited_class_reg_set_mode_p (enum re
(temp, ira_prohibited_class_mode_regs[rclass][mode]));
}
+
+/* Used to check validity info about small class input operands. It
+ should be incremented at start of processing an insn
+ alternative. */
+static unsigned int curr_small_class_check = 0;
+
+/* Update number of used inputs of class OP_CLASS for operand NOP.
+ Return true if we have more such class operands than the number of
+ available regs. */
+static bool
+update_and_check_small_class_inputs (int nop, enum reg_class op_class)
+{
+ static unsigned int small_class_check[LIM_REG_CLASSES];
+ static int small_class_input_nums[LIM_REG_CLASSES];
+
+ if (SMALL_REGISTER_CLASS_P (op_class)
+ /* We are interesting in classes became small because of fixing
+ some hard regs, e.g. by an user through GCC options. */
+ && hard_reg_set_intersect_p (reg_class_contents[op_class],
+ ira_no_alloc_regs)
+ && (curr_static_id->operand[nop].type != OP_OUT
+ || curr_static_id->operand[nop].early_clobber))
+ {
+ if (small_class_check[op_class] == curr_small_class_check)
+ small_class_input_nums[op_class]++;
+ else
+ {
+ small_class_check[op_class] = curr_small_class_check;
+ small_class_input_nums[op_class] = 1;
+ }
+ if (small_class_input_nums[op_class] > ira_class_hard_regs_num[op_class])
+ return true;
+ }
+ return false;
+}
+
/* Major function to choose the current insn alternative and what
operands should be reloaded and how. If ONLY_ALTERNATIVE is not
negative we should consider only this alternative. Return false if
@@ -1952,6 +1988,7 @@ process_alt_operands (int only_alternati
if (!TEST_BIT (preferred, nalt))
continue;
+ curr_small_class_check++;
overall = losers = addr_losers = 0;
static_reject = reject = reload_nregs = reload_sum = 0;
for (nop = 0; nop < n_operands; nop++)
@@ -2685,6 +2722,21 @@ process_alt_operands (int only_alternati
}
}
+ /* When we use memory operand, the insn should read the
+ value from memory and even if we just wrote a value
+ into the memory it is costly in comparison with an
+ insn alternative which does not use memory
+ (e.g. register or immediate operand). */
+ if (no_regs_p && offmemok)
+ {
+ if (lra_dump_file != NULL)
+ fprintf
+ (lra_dump_file,
+ " Using memory insn operand %d: reject+=3\n",
+ nop);
+ reject += 3;
+ }
+
#ifdef SECONDARY_MEMORY_NEEDED
/* If reload requires moving value through secondary
memory, it will need one more insn at least. */
@@ -2747,6 +2799,14 @@ process_alt_operands (int only_alternati
goto fail;
}
+ if (update_and_check_small_class_inputs (nop, this_alternative))
+ {
+ if (lra_dump_file != NULL)
+ fprintf (lra_dump_file,
+ " alt=%d, not enough small class regs -- refuse\n",
+ nalt);
+ goto fail;
+ }
curr_alt[nop] = this_alternative;
COPY_HARD_REG_SET (curr_alt_set[nop], this_alternative_set);
curr_alt_win[nop] = this_alternative_win;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: One more path to fix PR70478
2017-04-10 15:05 One more path to fix PR70478 Vladimir Makarov
@ 2017-04-11 7:30 ` Christophe Lyon
2017-04-11 15:42 ` Vladimir Makarov
2017-04-11 19:43 ` Vladimir Makarov
0 siblings, 2 replies; 8+ messages in thread
From: Christophe Lyon @ 2017-04-11 7:30 UTC (permalink / raw)
To: Vladimir Makarov; +Cc: gcc-patches
Hi Vladimir,
On 10 April 2017 at 17:05, Vladimir Makarov <vmakarov@redhat.com> wrote:
> This is the second try to fix
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70478
>
> The first try patch triggered a latent bug and broke one Fortran testcase
> on x86-64.
>
> The patch was successfully bootstrapped on x86-64 and tested on x86-64,
> ppc64, and aarch64.
>
> Committed as rev. 246808.
>
>
This patch causes regression on arm*hf configurations:
Executed from: gcc.target/arm/arm.exp
gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times ldrh\\tr[0-9]+ 2
gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times strh\\tr[0-9]+ 2
gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
vld1\\.16\\t{d[0-9]+\\[[0-9]+\\]}, \\[r[0-9]+\\] 2
gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
vmov\\.f16\\tr[0-9]+, s[0-9]+ 4
gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
vmov\\.f16\\ts[0-9]+, r[0-9]+ 4
gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
vst1\\.16\\t{d[0-9]+\\[[0-9]+\\]}, \\[r[0-9]+\\] 2
See http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/246809/report-build-info.html
Is it just a matter of adjusting the testcases? (note that there is no
regression when forcing either:
-march=armv5t
-mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
in the runtestflags.
I would have to re--run the build/test manually to get the generated
code, let me know if it's needed.
Thanks,
Christophe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: One more path to fix PR70478
2017-04-11 7:30 ` Christophe Lyon
@ 2017-04-11 15:42 ` Vladimir Makarov
2017-04-11 16:26 ` Christophe Lyon
2017-04-11 17:27 ` Vladimir Makarov
2017-04-11 19:43 ` Vladimir Makarov
1 sibling, 2 replies; 8+ messages in thread
From: Vladimir Makarov @ 2017-04-11 15:42 UTC (permalink / raw)
To: Christophe Lyon; +Cc: gcc-patches
On 04/11/2017 03:30 AM, Christophe Lyon wrote:
> Hi Vladimir,
>
> On 10 April 2017 at 17:05, Vladimir Makarov <vmakarov@redhat.com> wrote:
>> This is the second try to fix
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70478
>>
>> The first try patch triggered a latent bug and broke one Fortran testcase
>> on x86-64.
>>
>> The patch was successfully bootstrapped on x86-64 and tested on x86-64,
>> ppc64, and aarch64.
>>
>> Committed as rev. 246808.
>>
>>
> I would have to re--run the build/test manually to get the generated
> code, let me know if it's needed.
Yes, Christophe. It would be helpful. I've tried to reproduce it but I
don't see the difference in the generated code.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: One more path to fix PR70478
2017-04-11 15:42 ` Vladimir Makarov
@ 2017-04-11 16:26 ` Christophe Lyon
2017-04-11 16:31 ` Ramana Radhakrishnan
2017-04-11 17:27 ` Vladimir Makarov
1 sibling, 1 reply; 8+ messages in thread
From: Christophe Lyon @ 2017-04-11 16:26 UTC (permalink / raw)
To: Vladimir Makarov; +Cc: gcc-patches
On 11 April 2017 at 17:42, Vladimir Makarov <vmakarov@redhat.com> wrote:
>
>
> On 04/11/2017 03:30 AM, Christophe Lyon wrote:
>>
>> Hi Vladimir,
>>
>> On 10 April 2017 at 17:05, Vladimir Makarov <vmakarov@redhat.com> wrote:
>>>
>>> This is the second try to fix
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70478
>>>
>>> The first try patch triggered a latent bug and broke one Fortran
>>> testcase
>>> on x86-64.
>>>
>>> The patch was successfully bootstrapped on x86-64 and tested on
>>> x86-64,
>>> ppc64, and aarch64.
>>>
>>> Committed as rev. 246808.
>>>
>>>
>> I would have to re--run the build/test manually to get the generated
>> code, let me know if it's needed.
>
> Yes, Christophe. It would be helpful. I've tried to reproduce it but I
> don't see the difference in the generated code.
>
Here is what I observed (the "with-patch file is with your commit r246808,
the other is r246807)
--- armv8_2-fp16-move-1.s 2017-04-11 16:23:46.795264234 +0000
+++ armv8_2-fp16-move-1.s.with-patch 2017-04-11 15:54:52.563210963 +0000
@@ -37,8 +37,8 @@
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
lsl r1, r1, #1
- add r3, r0, r1
- vld1.16 {d0[0]}, [r3]
+ ldrh r3, [r0, r1] @ __fp16
+ vmov.f16 s0, r3 @ __fp16
bx lr
.size test_load_2, .-test_load_2
.align 2
@@ -64,9 +64,9 @@
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
+ vmov.f16 r3, s0 @ __fp16
lsl r1, r1, #1
- add r3, r0, r1
- vst1.16 {d0[0]}, [r3]
+ strh r3, [r0, r1] @ __fp16
bx lr
.size test_store_2, .-test_store_2
.align 2
Christophe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: One more path to fix PR70478
2017-04-11 16:26 ` Christophe Lyon
@ 2017-04-11 16:31 ` Ramana Radhakrishnan
0 siblings, 0 replies; 8+ messages in thread
From: Ramana Radhakrishnan @ 2017-04-11 16:31 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Vladimir Makarov, gcc-patches
On Tue, Apr 11, 2017 at 5:26 PM, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> On 11 April 2017 at 17:42, Vladimir Makarov <vmakarov@redhat.com> wrote:
>>
>>
>> On 04/11/2017 03:30 AM, Christophe Lyon wrote:
>>>
>>> Hi Vladimir,
>>>
>>> On 10 April 2017 at 17:05, Vladimir Makarov <vmakarov@redhat.com> wrote:
>>>>
>>>> This is the second try to fix
>>>>
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70478
>>>>
>>>> The first try patch triggered a latent bug and broke one Fortran
>>>> testcase
>>>> on x86-64.
>>>>
>>>> The patch was successfully bootstrapped on x86-64 and tested on
>>>> x86-64,
>>>> ppc64, and aarch64.
>>>>
>>>> Committed as rev. 246808.
>>>>
>>>>
>>> I would have to re--run the build/test manually to get the generated
>>> code, let me know if it's needed.
>>
>> Yes, Christophe. It would be helpful. I've tried to reproduce it but I
>> don't see the difference in the generated code.
>>
>
> Here is what I observed (the "with-patch file is with your commit r246808,
> the other is r246807)
>
> --- armv8_2-fp16-move-1.s 2017-04-11 16:23:46.795264234 +0000
> +++ armv8_2-fp16-move-1.s.with-patch 2017-04-11 15:54:52.563210963 +0000
> @@ -37,8 +37,8 @@
> @ frame_needed = 0, uses_anonymous_args = 0
> @ link register save eliminated.
> lsl r1, r1, #1
> - add r3, r0, r1
> - vld1.16 {d0[0]}, [r3]
> + ldrh r3, [r0, r1] @ __fp16
> + vmov.f16 s0, r3 @ __fp16
> bx lr
> .size test_load_2, .-test_load_2
> .align 2
> @@ -64,9 +64,9 @@
> @ args = 0, pretend = 0, frame = 0
> @ frame_needed = 0, uses_anonymous_args = 0
> @ link register save eliminated.
> + vmov.f16 r3, s0 @ __fp16
> lsl r1, r1, #1
> - add r3, r0, r1
> - vst1.16 {d0[0]}, [r3]
> + strh r3, [r0, r1] @ __fp16
> bx lr
> .size test_store_2, .-test_store_2
> .align 2
>
That's actually bad because we've now introduced additional moves
between the integer and FP register files. It could be something in
the backend but this is worth investigating further
Ramana
>
> Christophe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: One more path to fix PR70478
2017-04-11 15:42 ` Vladimir Makarov
2017-04-11 16:26 ` Christophe Lyon
@ 2017-04-11 17:27 ` Vladimir Makarov
1 sibling, 0 replies; 8+ messages in thread
From: Vladimir Makarov @ 2017-04-11 17:27 UTC (permalink / raw)
To: Christophe Lyon; +Cc: gcc-patches
On 04/11/2017 11:42 AM, Vladimir Makarov wrote:
>
>
> Yes, Christophe. It would be helpful. I've tried to reproduce it but
> I don't see the difference in the generated code.
>
Never mind. I've reproduced it. Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: One more path to fix PR70478
2017-04-11 7:30 ` Christophe Lyon
2017-04-11 15:42 ` Vladimir Makarov
@ 2017-04-11 19:43 ` Vladimir Makarov
2017-04-12 6:10 ` Christophe Lyon
1 sibling, 1 reply; 8+ messages in thread
From: Vladimir Makarov @ 2017-04-11 19:43 UTC (permalink / raw)
To: Christophe Lyon; +Cc: gcc-patches
On 04/11/2017 03:30 AM, Christophe Lyon wrote:
> Hi Vladimir,
>
> On 10 April 2017 at 17:05, Vladimir Makarov <vmakarov@redhat.com> wrote:
>> This is the second try to fix
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70478
>>
>> The first try patch triggered a latent bug and broke one Fortran testcase
>> on x86-64.
>>
>> The patch was successfully bootstrapped on x86-64 and tested on x86-64,
>> ppc64, and aarch64.
>>
>> Committed as rev. 246808.
>>
>>
> This patch causes regression on arm*hf configurations:
> Executed from: gcc.target/arm/arm.exp
> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times ldrh\\tr[0-9]+ 2
> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times strh\\tr[0-9]+ 2
> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
> vld1\\.16\\t{d[0-9]+\\[[0-9]+\\]}, \\[r[0-9]+\\] 2
> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
> vmov\\.f16\\tr[0-9]+, s[0-9]+ 4
> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
> vmov\\.f16\\ts[0-9]+, r[0-9]+ 4
> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
> vst1\\.16\\t{d[0-9]+\\[[0-9]+\\]}, \\[r[0-9]+\\] 2
>
>
I've committed a patch which is supposed to fix the regression.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: One more path to fix PR70478
2017-04-11 19:43 ` Vladimir Makarov
@ 2017-04-12 6:10 ` Christophe Lyon
0 siblings, 0 replies; 8+ messages in thread
From: Christophe Lyon @ 2017-04-12 6:10 UTC (permalink / raw)
To: Vladimir Makarov; +Cc: gcc-patches
On 11 April 2017 at 21:43, Vladimir Makarov <vmakarov@redhat.com> wrote:
>
>
> On 04/11/2017 03:30 AM, Christophe Lyon wrote:
>>
>> Hi Vladimir,
>>
>> On 10 April 2017 at 17:05, Vladimir Makarov <vmakarov@redhat.com> wrote:
>>>
>>> This is the second try to fix
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70478
>>>
>>> The first try patch triggered a latent bug and broke one Fortran
>>> testcase
>>> on x86-64.
>>>
>>> The patch was successfully bootstrapped on x86-64 and tested on
>>> x86-64,
>>> ppc64, and aarch64.
>>>
>>> Committed as rev. 246808.
>>>
>>>
>> This patch causes regression on arm*hf configurations:
>> Executed from: gcc.target/arm/arm.exp
>> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
>> ldrh\\tr[0-9]+ 2
>> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
>> strh\\tr[0-9]+ 2
>> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
>> vld1\\.16\\t{d[0-9]+\\[[0-9]+\\]}, \\[r[0-9]+\\] 2
>> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
>> vmov\\.f16\\tr[0-9]+, s[0-9]+ 4
>> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
>> vmov\\.f16\\ts[0-9]+, r[0-9]+ 4
>> gcc.target/arm/armv8_2-fp16-move-1.c scan-assembler-times
>> vst1\\.16\\t{d[0-9]+\\[[0-9]+\\]}, \\[r[0-9]+\\] 2
>>
>>
> I've committed a patch which is supposed to fix the regression.
>
I confirm it's now OK. Thanks for the prompt fix!
Christophe
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-12 6:10 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-10 15:05 One more path to fix PR70478 Vladimir Makarov
2017-04-11 7:30 ` Christophe Lyon
2017-04-11 15:42 ` Vladimir Makarov
2017-04-11 16:26 ` Christophe Lyon
2017-04-11 16:31 ` Ramana Radhakrishnan
2017-04-11 17:27 ` Vladimir Makarov
2017-04-11 19:43 ` Vladimir Makarov
2017-04-12 6:10 ` Christophe Lyon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).