* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
@ 2021-03-23 2:00 ` luoxhu at gcc dot gnu.org
2021-03-23 7:26 ` rguenth at gcc dot gnu.org
` (20 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: luoxhu at gcc dot gnu.org @ 2021-03-23 2:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
luoxhu at gcc dot gnu.org changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |dje.gcc at gmail dot com,
| |segher at gcc dot gnu.org
--- Comment #1 from luoxhu at gcc dot gnu.org ---
Confirmed. Another case forgot to test m32 again :(
David mentioned no need variable vec_insert support for m32 build, so I think
we should avoid generating IFN VEC_SET in
gimple-isel.c:gimple_expand_vec_set_expr, but it seems not possible to check
"TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT" in the common file or through
can_vec_set_var_idx_p. Any suggestions?
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564403.html
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
2021-03-23 2:00 ` [Bug target/99718] " luoxhu at gcc dot gnu.org
@ 2021-03-23 7:26 ` rguenth at gcc dot gnu.org
2021-03-23 7:26 ` rguenth at gcc dot gnu.org
` (19 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: rguenth at gcc dot gnu.org @ 2021-03-23 7:26 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|--- |11.0
--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to luoxhu from comment #1)
> Confirmed. Another case forgot to test m32 again :(
>
> David mentioned no need variable vec_insert support for m32 build, so I
> think we should avoid generating IFN VEC_SET in
> gimple-isel.c:gimple_expand_vec_set_expr, but it seems not possible to check
> "TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT" in the common file or through
> can_vec_set_var_idx_p. Any suggestions?
>
> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564403.html
You can for sure add that to the appropriate insn condition?
But then why should vec_set not be implemented for -m32?
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
2021-03-23 2:00 ` [Bug target/99718] " luoxhu at gcc dot gnu.org
2021-03-23 7:26 ` rguenth at gcc dot gnu.org
@ 2021-03-23 7:26 ` rguenth at gcc dot gnu.org
2021-03-23 12:44 ` jakub at gcc dot gnu.org
` (18 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: rguenth at gcc dot gnu.org @ 2021-03-23 7:26 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Priority|P3 |P1
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (2 preceding siblings ...)
2021-03-23 7:26 ` rguenth at gcc dot gnu.org
@ 2021-03-23 12:44 ` jakub at gcc dot gnu.org
2021-03-24 1:57 ` luoxhu at gcc dot gnu.org
` (17 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-03-23 12:44 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jakub at gcc dot gnu.org
--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to luoxhu from comment #1)
> Confirmed. Another case forgot to test m32 again :(
>
> David mentioned no need variable vec_insert support for m32 build, so I
> think we should avoid generating IFN VEC_SET in
> gimple-isel.c:gimple_expand_vec_set_expr, but it seems not possible to check
> "TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT" in the common file or through
> can_vec_set_var_idx_p. Any suggestions?
>
> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564403.html
If the non-constant vec_set can't be supported when
!(TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT)
then
(define_expand "vec_set<mode>"
[(match_operand:VEC_E 0 "vlogical_operand")
(match_operand:<VEC_base> 1 "register_operand")
(match_operand 2 "reg_or_cint_operand")]
"VECTOR_MEM_ALTIVEC_OR_VSX_P (<MODE>mode)"
{
rs6000_expand_vector_set (operands[0], operands[1], operands[2]);
DONE;
})
should be changed, perhaps add a new predicate that for
TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT
is equivalent to reg_or_cint_operand and otherwise to const_int_operand?
(define_predicate "vec_set_index_operand"
(if_then_else (match_test "TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT")
(match_operand 0 "reg_or_cint_operand")
(match_operand 0 "const_int_operand")))
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (3 preceding siblings ...)
2021-03-23 12:44 ` jakub at gcc dot gnu.org
@ 2021-03-24 1:57 ` luoxhu at gcc dot gnu.org
2021-03-24 20:02 ` segher at gcc dot gnu.org
` (16 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: luoxhu at gcc dot gnu.org @ 2021-03-24 1:57 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #4 from luoxhu at gcc dot gnu.org ---
Thanks, Jakub. It tested pass on both m32/m64, is this a reasonable fix?
@segher, will make it a patch if so.
git diff
diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
index 859af75..0a5cae2 100644
--- a/gcc/config/rs6000/predicates.md
+++ b/gcc/config/rs6000/predicates.md
@@ -1920,6 +1920,12 @@
return address_is_prefixed (XEXP (op, 0), mode, NON_PREFIXED_DEFAULT);
})
+;; Return true if m64 on p8v and above for vec_set with variable index.
+(define_predicate "vec_set_index_operand"
+ (if_then_else (match_test "TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT")
+ (match_operand 0 "reg_or_cint_operand")
+ (match_operand 0 "const_int_operand")))
+
;; Return true if the operand is a valid memory operand with a D-form
;; address that could be merged with the load of a PC-relative external
address
;; with the PCREL_OPT optimization. We don't check here whether or not the
diff --git a/gcc/config/rs6000/vector.md b/gcc/config/rs6000/vector.md
index e5191bd..3446b03 100644
--- a/gcc/config/rs6000/vector.md
+++ b/gcc/config/rs6000/vector.md
@@ -1227,7 +1227,7 @@
(define_expand "vec_set<mode>"
[(match_operand:VEC_E 0 "vlogical_operand")
(match_operand:<VEC_base> 1 "register_operand")
- (match_operand 2 "reg_or_cint_operand")]
+ (match_operand 2 "vec_set_index_operand")]
"VECTOR_MEM_ALTIVEC_OR_VSX_P (<MODE>mode)"
{
rs6000_expand_vector_set (operands[0], operands[1], operands[2]);
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (4 preceding siblings ...)
2021-03-24 1:57 ` luoxhu at gcc dot gnu.org
@ 2021-03-24 20:02 ` segher at gcc dot gnu.org
2021-03-25 15:58 ` jakub at gcc dot gnu.org
` (15 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: segher at gcc dot gnu.org @ 2021-03-24 20:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #5 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #3)
> If the non-constant vec_set can't be supported when
> !(TARGET_P8_VECTOR && TARGET_DIRECT_MOVE_64BIT)
I don't see why not? It may need different code, sure, but that is much
preferable over contorting the rest of the backend.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (5 preceding siblings ...)
2021-03-24 20:02 ` segher at gcc dot gnu.org
@ 2021-03-25 15:58 ` jakub at gcc dot gnu.org
2021-03-25 16:31 ` segher at gcc dot gnu.org
` (14 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-03-25 15:58 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I did not know whether it is implementable (in VSX or in Altivec) for 32-bit
targets etc., all I was suggesting was what to do if it is not implementable.
If it is implementable, somebody familiar with VSX/Altivec should add the
implementation, or we can temporarily use the patch that has been posted and
get back to it later. Or if it is partly implementable (e.g. can be done in
VSX and can't be done in Altivec, etc.), then the patch can still be used after
amendments for what will and what will not work.
Right now it is a P1 blocker because we ICE on something that worked perfectly
fine (perhaps slower than it could) in GCC 10. So something needs to be done
before GCC 11 and we have ~ a month left for that.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (6 preceding siblings ...)
2021-03-25 15:58 ` jakub at gcc dot gnu.org
@ 2021-03-25 16:31 ` segher at gcc dot gnu.org
2021-03-25 16:37 ` dje at gcc dot gnu.org
` (13 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: segher at gcc dot gnu.org @ 2021-03-25 16:31 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #7 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #6)
> I did not know whether it is implementable (in VSX or in Altivec) for 32-bit
> targets etc., all I was suggesting was what to do if it is not implementable.
Yes.
> If it is implementable, somebody familiar with VSX/Altivec should add the
> implementation, or we can temporarily use the patch that has been posted and
> get back to it later.
I haven't seen a patch posted yet?
> Or if it is partly implementable (e.g. can be done in
> VSX and can't be done in Altivec, etc.), then the patch can still be used
> after amendments for what will and what will not work.
The only thing I am saying it should be massively easier to just implement it
for -m32 as well, much easier than adding extra conditions (and unavoidably
getting that wrong).
> Right now it is a P1 blocker because we ICE on something that worked
> perfectly fine (perhaps slower than it could) in GCC 10. So something needs
> to be done before GCC 11 and we have ~ a month left for that.
Yup.
I'll review any patch that is sent (cc: me, so that I see it immediately,
instead of after 3 to 6 weeks).
Thanks,
Segher
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (7 preceding siblings ...)
2021-03-25 16:31 ` segher at gcc dot gnu.org
@ 2021-03-25 16:37 ` dje at gcc dot gnu.org
2021-03-25 16:38 ` dje at gcc dot gnu.org
` (12 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: dje at gcc dot gnu.org @ 2021-03-25 16:37 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #8 from David Edelsohn <dje at gcc dot gnu.org> ---
Xionghu, please do not write "can't" when you mean "it's difficult" or "it
hasn't been implemented" or "it's too inefficient" (such as moving the data
through memory). Please be very precise in your explanations.
I never wrote that there is no need variable vec_insert support for m32 build.
Does LLVM generate good code for this operation in 32 bit mode?
As Jakub said, this is a P1 blocker. We may want to fix this differently in
the short term than the long term. We may want to TEMPORARILY avoid this
situation for m32 configuration for the upcoming release but GCC should
generate a better instruction sequence in the next release cycle.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (8 preceding siblings ...)
2021-03-25 16:37 ` dje at gcc dot gnu.org
@ 2021-03-25 16:38 ` dje at gcc dot gnu.org
2021-03-25 16:55 ` jakub at gcc dot gnu.org
` (11 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: dje at gcc dot gnu.org @ 2021-03-25 16:38 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
David Edelsohn <dje at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |NEW
CC| |dje at gcc dot gnu.org
Last reconfirmed| |2021-03-25
Ever confirmed|0 |1
--- Comment #9 from David Edelsohn <dje at gcc dot gnu.org> ---
Confirmed.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (9 preceding siblings ...)
2021-03-25 16:38 ` dje at gcc dot gnu.org
@ 2021-03-25 16:55 ` jakub at gcc dot gnu.org
2021-03-26 5:37 ` luoxhu at gcc dot gnu.org
` (10 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-03-25 16:55 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567215.html
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (10 preceding siblings ...)
2021-03-25 16:55 ` jakub at gcc dot gnu.org
@ 2021-03-26 5:37 ` luoxhu at gcc dot gnu.org
2021-03-26 5:38 ` luoxhu at gcc dot gnu.org
` (9 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: luoxhu at gcc dot gnu.org @ 2021-03-26 5:37 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #11 from luoxhu at gcc dot gnu.org ---
Created attachment 50474
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50474&action=edit
32bit variable vec_insert
LLVM also generates store-hit-load instruction:
addi 3, 1, -16
rlwinm 4, 5, 2, 28, 29
stvx 2, 0, 3
stwx 6, 3, 4
lvx 2, 0, 3
blr
.long 0
.quad 0
I didn't use "can't" in my reply, sorry that caused the confusion, we though it
was inefficient to move SF to SI on 32bit mode , but it turns out also huge
performance gain (46.704s -> 4.369s).
Attached the patch that also support variable vec_insert for 32bit, testing on
P8BE/PBLE/P9LE, could you please verify it on AIX? Will refine it and send to
the mail-list to fix this P1 issue fundamentally.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (11 preceding siblings ...)
2021-03-26 5:37 ` luoxhu at gcc dot gnu.org
@ 2021-03-26 5:38 ` luoxhu at gcc dot gnu.org
2021-03-26 6:14 ` luoxhu at gcc dot gnu.org
` (8 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: luoxhu at gcc dot gnu.org @ 2021-03-26 5:38 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #12 from luoxhu at gcc dot gnu.org ---
Not sure whether TARGET_DIRECT_MOVE_64BIT is the right MACRO to correctly
differentiate m32 and m64?
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (12 preceding siblings ...)
2021-03-26 5:38 ` luoxhu at gcc dot gnu.org
@ 2021-03-26 6:14 ` luoxhu at gcc dot gnu.org
2021-03-26 8:22 ` jakub at gcc dot gnu.org
` (7 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: luoxhu at gcc dot gnu.org @ 2021-03-26 6:14 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #13 from luoxhu at gcc dot gnu.org ---
Performance data in #c11 is for int variable vec_insert of 32bit mode, the
float variable vec_insert of 32-bit is a bit slower but much better than
original(extra stfs+lwz of insn #17 and insn 18 in expand to move SF register
to SI register by hex value.):
46.677s -> 8.723s
test.c
#include <altivec.h>
#define TYPE float
vector TYPE
test (vector TYPE u, TYPE i, signed int n){
return vec_insert (i, u, n);
}
Expand:
1: NOTE_INSN_DELETED
6: NOTE_INSN_BASIC_BLOCK 2
2: r122:V4SF=%2:V4SF
3: r123:SF=%1:SF
4: r124:SI=%3:SI
5: NOTE_INSN_FUNCTION_BEG
8: r120:V4SF=r122:V4SF
9: r125:SI=r124:SI&0x3
10: r126:V4SF=r120:V4SF
11: r128:SI=r125:SI<<0x2
12: {r128:SI=0x14-r128:SI;clobber ca:SI;}
13: r132:SI=high(`*.LC0')
14: r131:SI=r132:SI+low(`*.LC0')
REG_EQUAL `*.LC0'
15: r130:V2DI=[r131:SI]
REG_EQUAL const_vector
16: r129:V16QI=r130:V2DI#0
17: [r112:SI]=r123:SF
18: r133:SI=[r112:SI]
19: r136:DI#4=r133:SI
22: {r137:SI=r133:SI>>0x1f;clobber ca:SI;}
23: r136:DI#0=r137:SI
24: r138:DI=0
25: r135:V2DI=vec_concat(r136:DI,r138:DI)
26: r134:V16QI=r135:V2DI#0
27: r139:V16QI=unspec[r128:SI] 151
28: r140:V16QI=unspec[r134:V16QI,r134:V16QI,r139:V16QI] 236
29: r141:V16QI=unspec[r129:V16QI,r129:V16QI,r139:V16QI] 236
30: r126:V4SF#0={(r141:V16QI!=const_vector)?r140:V16QI:r126:V4SF#0}
31: r119:V4SF=r126:V4SF
32: r120:V4SF=r119:V4SF
ASM:
.LFB0:
.cfi_startproc
stwu 1,-16(1)
.cfi_def_cfa_offset 16
lis 9,.LC0@ha
rlwinm 3,3,2,28,29
xxlxor 0,0,0
la 9,.LC0@l(9)
subfic 3,3,20
lxvd2x 33,0,9
lvsl 13,0,3
stfs 1,8(1)
vperm 1,1,1,13
ori 2,2,0
lwz 9,8(1)
addi 1,1,16
.cfi_def_cfa_offset 0
srawi 10,9,31
mtvsrwz 13,9
mtvsrwz 12,10
fmrgow 11,12,13
xxpermdi 32,11,0,0
vperm 0,0,0,13
xxsel 34,34,32,33
blr
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (13 preceding siblings ...)
2021-03-26 6:14 ` luoxhu at gcc dot gnu.org
@ 2021-03-26 8:22 ` jakub at gcc dot gnu.org
2021-03-26 12:27 ` luoxhu at gcc dot gnu.org
` (6 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-03-26 8:22 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
You still have:
if (VECTOR_MEM_VSX_P (mode))
{
if (!CONST_INT_P (elt_rtx))
{
if ((TARGET_P9_VECTOR && TARGET_POWERPC64) || width == 8)
return ..._p9 (...);
else if (TARGET_P8_VECTOR)
return ..._p8 (...);
}
if (mode == V2DFmode)
insn = gen_vsx_set_v2df (target, target, val, elt_rtx);
else if (mode == V2DImode)
insn = gen_vsx_set_v2di (target, target, val, elt_rtx);
else if (TARGET_P9_VECTOR && TARGET_POWERPC64)
{
...
}
if (insn)
return;
}
gcc_assert (CONST_INT_P (elt_rtx));
while the vector.md condition is VECTOR_MEM_ALTIVEC_OR_VSX_P (<MODE>mode),
i.e. true for TARGET_ALTIVEC for many modes already (V4SI, V8HI, V16QI, V4SF
and
for TARGET_VSX also V2DF and V2DI, right).
I somehow don't see how this can work properly.
Looking at vsx_set_v2df and vsx_set_v2di, neither of them will handle
non-constant elt_rtx (it ICEs on anything but const0_rtx and const1_rtx).
So, questions:
1) does the rs6000_expand_vector_set_var_p9 routine for width == 8 (i.e.
V2DImode or V2DFmode?)
handle everything, even when TARGET_P9_VECTOR or TARGET_POWERPC64 is not true,
plain old VSX?
2) what happens if TARGET_P8_VECTOR is false and TARGET_VSX is true and mode is
other than V2DI/V2DF? If I read the code right, it will fall through to
gcc_assert (CONST_INT_P (elt_rtx));
3) what happens if !TARGET_VSX (more specifically, when VECTOR_MEM_VSX_P (mode)
is false.
I see there just the assertion that would fail right away.
Perhaps I'm missing something obvious and those cases are impossible, but if
that is the case, it would still be better to add further assertion at least to
the if (...) else if (...) as else gcc_assert ...
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (14 preceding siblings ...)
2021-03-26 8:22 ` jakub at gcc dot gnu.org
@ 2021-03-26 12:27 ` luoxhu at gcc dot gnu.org
2021-03-26 13:26 ` jakub at gcc dot gnu.org
` (5 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: luoxhu at gcc dot gnu.org @ 2021-03-26 12:27 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #15 from luoxhu at gcc dot gnu.org ---
(In reply to Jakub Jelinek from comment #14)
> You still have:
> if (VECTOR_MEM_VSX_P (mode))
> {
> if (!CONST_INT_P (elt_rtx))
> {
> if ((TARGET_P9_VECTOR && TARGET_POWERPC64) || width == 8)
> return ..._p9 (...);
> else if (TARGET_P8_VECTOR)
> return ..._p8 (...);
> }
>
> if (mode == V2DFmode)
> insn = gen_vsx_set_v2df (target, target, val, elt_rtx);
>
> else if (mode == V2DImode)
> insn = gen_vsx_set_v2di (target, target, val, elt_rtx);
>
> else if (TARGET_P9_VECTOR && TARGET_POWERPC64)
> {
> ...
> }
> if (insn)
> return;
> }
>
> gcc_assert (CONST_INT_P (elt_rtx));
>
> while the vector.md condition is VECTOR_MEM_ALTIVEC_OR_VSX_P (<MODE>mode),
> i.e. true for TARGET_ALTIVEC for many modes already (V4SI, V8HI, V16QI, V4SF
> and
> for TARGET_VSX also V2DF and V2DI, right).
> I somehow don't see how this can work properly.
> Looking at vsx_set_v2df and vsx_set_v2di, neither of them will handle
> non-constant elt_rtx (it ICEs on anything but const0_rtx and const1_rtx).
>
> So, questions:
> 1) does the rs6000_expand_vector_set_var_p9 routine for width == 8 (i.e.
> V2DImode or V2DFmode?)
> handle everything, even when TARGET_P9_VECTOR or TARGET_POWERPC64 is not
> true, plain old VSX?
Yes. V2DI/V2DF for P8 {BE,LE} {m32,m64} will call
rs6000_expand_vector_set_var_p9 instead of xxx_p8.
Do you mean Power7 for the plain old VSX? I verified the pr98914.c on Power7,
it exactly ICEs on "gcc_assert (CONST_INT_P (elt_rtx));" for both m64 and m32.
This is still not fixed by the patch in #c11 yet.
For builtin call in rs6000-c.c:altivec_build_resolved_builtin, it is guarded by
TARGET_P8_VECTOR, so Power7 doesn't generate IFN VEC_INSERT before. This ICE
also comes from internal optimization gimple-isel.c:gimple_expand_vec_set_expr,
can_vec_set_var_idx_p doesn't return false due to VECTOR_MEM_ALTIVEC_OR_VSX_P
is true when Power7 VSX, change the "if (VECTOR_MEM_VSX_P (mode))" to "if
(VECTOR_MEM_ALTIVEC_OR_VSX_P (mode))" in rs6000.c:rs6000_expand_vector_set and
remove TARGET_P8_VECTOR in the else branch could fix the ICE on P7 {m32,64}, so
this means even P7 VSX could benefit from this optimization, which is different
from what discussed before.
> 2) what happens if TARGET_P8_VECTOR is false and TARGET_VSX is true and mode
> is other than V2DI/V2DF? If I read the code right, it will fall through to
> gcc_assert (CONST_INT_P (elt_rtx));
Same like 1)?
> 3) what happens if !TARGET_VSX (more specifically, when VECTOR_MEM_VSX_P
> (mode) is false.
> I see there just the assertion that would fail right away.
> Perhaps I'm missing something obvious and those cases are impossible, but if
> that is the case, it would still be better to add further assertion at least
> to the if (...) else if (...) as else gcc_assert ...
Thanks for pointing out, the "gcc_assert (CONST_INT_P (elt_rtx));" should be
moved into the "if (!CONST_INT_P (elt_rtx))" condition like you said.
gen_vsx_set_v2df and gen_vsx_set_v2di are supposed to handle only const
elt_rtx.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (15 preceding siblings ...)
2021-03-26 12:27 ` luoxhu at gcc dot gnu.org
@ 2021-03-26 13:26 ` jakub at gcc dot gnu.org
2021-03-26 22:39 ` segher at gcc dot gnu.org
` (4 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-03-26 13:26 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to luoxhu from comment #15)
> Do you mean Power7 for the plain old VSX? I verified the pr98914.c on
> Power7, it exactly ICEs on "gcc_assert (CONST_INT_P (elt_rtx));" for both
> m64 and m32. This is still not fixed by the patch in #c11 yet.
Sure, any CPU that has VSX or Altivec support.
So VSX for power7 and Altivec for power5/6/G4/G5 or whatever has Altivec
support.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (16 preceding siblings ...)
2021-03-26 13:26 ` jakub at gcc dot gnu.org
@ 2021-03-26 22:39 ` segher at gcc dot gnu.org
2021-03-26 22:51 ` segher at gcc dot gnu.org
` (3 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: segher at gcc dot gnu.org @ 2021-03-26 22:39 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #17 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #10)
> https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567215.html
Ah, that is more recent than anything I have replied to :-(
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (17 preceding siblings ...)
2021-03-26 22:39 ` segher at gcc dot gnu.org
@ 2021-03-26 22:51 ` segher at gcc dot gnu.org
2021-03-27 3:42 ` luoxhu at gcc dot gnu.org
` (2 subsequent siblings)
21 siblings, 0 replies; 23+ messages in thread
From: segher at gcc dot gnu.org @ 2021-03-26 22:51 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #18 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to luoxhu from comment #12)
> Not sure whether TARGET_DIRECT_MOVE_64BIT is the right MACRO to correctly
> differentiate m32 and m64?
It is not. It looks at TARGET_POWERPC64 only, and that can be set for -m32
just fine.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (18 preceding siblings ...)
2021-03-26 22:51 ` segher at gcc dot gnu.org
@ 2021-03-27 3:42 ` luoxhu at gcc dot gnu.org
2021-03-30 13:57 ` cvs-commit at gcc dot gnu.org
2021-03-31 0:55 ` luoxhu at gcc dot gnu.org
21 siblings, 0 replies; 23+ messages in thread
From: luoxhu at gcc dot gnu.org @ 2021-03-27 3:42 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #19 from luoxhu at gcc dot gnu.org ---
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567395.html
This patch extends variable vec_insert to all 32bit VSX targets including
Power7{BE} {32,64}, Power8{BE}{32, 64}, Power8{LE}{64}, Power9{LE}{64}, all
tested pass for power testcases, though AIX is not tested yet. @Segher, please
review this one instead of the previous that disables 32 bit variable
vec_insert, thanks.
For Altivec targets like power5/6/G4/G5, take the previous "vector store/scalar
store/vector load" code path.
-mcpu=power6 -O2 -maltivec -c -S
f2:
.LFB0:
.cfi_startproc
addi 10,1,-16
sldi 5,5,2
li 9,32
addi 8,1,-48
stvx 2,8,9
stwx 6,10,5
lvx 2,8,9
blr
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (19 preceding siblings ...)
2021-03-27 3:42 ` luoxhu at gcc dot gnu.org
@ 2021-03-30 13:57 ` cvs-commit at gcc dot gnu.org
2021-03-31 0:55 ` luoxhu at gcc dot gnu.org
21 siblings, 0 replies; 23+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-03-30 13:57 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
--- Comment #20 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Segher Boessenkool <segher@gcc.gnu.org>:
https://gcc.gnu.org/g:f64b91568f3ac8f152c6c617b4fcc6b51da10ac4
commit r11-7911-gf64b91568f3ac8f152c6c617b4fcc6b51da10ac4
Author: luoxhu@cn.ibm.com <luoxhu@cn.ibm.com>
Date: Fri Mar 26 22:26:57 2021 -0500
rs6000: Enable 32bit variable vec_insert [PR99718]
32bit and P7 VSX could also benefit a lot from the variable vec_insert
implementation with shift/insert/shift back method.
2011-03-29 Xionghu Luo <luoxhu@linux.ibm.com>
PR target/99718
* config/rs6000/altivec.md (altivec_lvsl_reg): Change to ...
(altivec_lvsl_reg_<mode>): ... this.
(altivec_lvsr_reg): Change to ...
(altivec_lvsr_reg_<mode>): ... this.
* config/rs6000/predicates.md (vec_set_index_operand): New.
* config/rs6000/rs6000-c.c (altivec_resolve_overloaded_builtin):
Enable 32bit variable vec_insert for all TARGET_VSX.
* config/rs6000/rs6000.c (rs6000_expand_vector_set_var_p9):
Enable 32bit variable vec_insert for p9 and above.
(rs6000_expand_vector_set_var_p8): Rename to ...
(rs6000_expand_vector_set_var_p7): ... this.
(rs6000_expand_vector_set): Use TARGET_VSX and adjust assert
position.
* config/rs6000/vector.md (vec_set<mode>): Use
vec_set_index_operand.
* config/rs6000/vsx.md (xl_len_r): Use gen_altivec_lvsl_reg_di and
gen_altivec_lvsr_reg_di.
gcc/testsuite/
PR target/99718
* gcc.target/powerpc/fold-vec-insert-char-p8.c: Update
instruction counts.
* gcc.target/powerpc/fold-vec-insert-char-p9.c: Likewise.
* gcc.target/powerpc/fold-vec-insert-double.c: Likewise.
* gcc.target/powerpc/fold-vec-insert-float-p8.c: Likewise.
* gcc.target/powerpc/fold-vec-insert-float-p9.c: Likewise.
* gcc.target/powerpc/fold-vec-insert-int-p8.c: Likewise.
* gcc.target/powerpc/fold-vec-insert-int-p9.c: Likewise.
* gcc.target/powerpc/fold-vec-insert-longlong.c: Likewise.
* gcc.target/powerpc/fold-vec-insert-short-p8.c: Likewise.
* gcc.target/powerpc/fold-vec-insert-short-p9.c: Likewise.
* gcc.target/powerpc/pr79251.p8.c: Likewise.
* gcc.target/powerpc/pr79251.p9.c: Likewise.
* gcc.target/powerpc/vsx-builtin-7.c: Likewise.
* gcc.target/powerpc/pr79251-run.p7.c: New test.
* gcc.target/powerpc/pr79251.p7.c: New test.
^ permalink raw reply [flat|nested] 23+ messages in thread* [Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits
2021-03-22 16:32 [Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits seurer at gcc dot gnu.org
` (20 preceding siblings ...)
2021-03-30 13:57 ` cvs-commit at gcc dot gnu.org
@ 2021-03-31 0:55 ` luoxhu at gcc dot gnu.org
21 siblings, 0 replies; 23+ messages in thread
From: luoxhu at gcc dot gnu.org @ 2021-03-31 0:55 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718
luoxhu at gcc dot gnu.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #21 from luoxhu at gcc dot gnu.org ---
Fixed on mater.
^ permalink raw reply [flat|nested] 23+ messages in thread