[Bug target/99718] New: [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[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]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99718

            Bug ID: 99718
           Summary: [11 regression] ICE in new test case
                    gcc.target/powerpc/pr98914.c  for 32 bits
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: seurer at gcc dot gnu.org
  Target Milestone: ---

g:d0a5e8e1a84bdd6ce915c3be65da8af2552cd49e, r11-7755
make  -k check-gcc RUNTESTFLAGS="--target_board=unix'{-m32}'
powerpc.exp=gcc.target/powerpc/pr98914.c"
FAIL: gcc.target/powerpc/pr98914.c (internal compiler error)
FAIL: gcc.target/powerpc/pr98914.c (test for excess errors)
# of unexpected failures        2

This fails for 32 bits only on powerpc64 BE.

spawn /home/seurer/gcc/git/build/gcc-test/gcc/xgcc
-B/home/seurer/gcc/git/build/gcc-test/gcc/ exceptions_enabled3605.cc
-fdiagnostics-plain-output -S -m32 -o exceptions_enabled3605.s
FAIL: gcc.target/powerpc/pr98914.c (test for excess errors)
Excess errors:
during RTL pass: expand
/home/seurer/gcc/git/gcc-test/gcc/testsuite/gcc.target/powerpc/pr98914.c:9:10:
internal compiler error: in rs6000_expand_vector_set, at
config/rs6000/rs6000.c:7221
0x1101a543 rs6000_expand_vector_set(rtx_def*, rtx_def*, rtx_def*)
        /home/seurer/gcc/git/gcc-test/gcc/config/rs6000/rs6000.c:7221
0x11631653 gen_vec_setv16qi(rtx_def*, rtx_def*, rtx_def*)
        /home/seurer/gcc/git/gcc-test/gcc/config/rs6000/vector.md:1233
0x1097519b rtx_insn* insn_gen_fn::operator()<rtx_def*, rtx_def*,
rtx_def*>(rtx_def*, rtx_def*, rtx_def*) const
        /home/seurer/gcc/git/gcc-test/gcc/recog.h:407
0x1097519b maybe_gen_insn(insn_code, unsigned int, expand_operand*)
        /home/seurer/gcc/git/gcc-test/gcc/optabs.c:7787
0x1097538f maybe_expand_insn(insn_code, unsigned int, expand_operand*)
        /home/seurer/gcc/git/gcc-test/gcc/optabs.c:7820
0x1077c017 expand_vec_set_optab_fn
        /home/seurer/gcc/git/gcc-test/gcc/internal-fn.c:2871
0x1077c017 expand_VEC_SET
        /home/seurer/gcc/git/gcc-test/gcc/internal-fn.def:148
0x1078a82f expand_internal_call(internal_fn, gcall*)
        /home/seurer/gcc/git/gcc-test/gcc/internal-fn.c:4079
0x1078a82f expand_internal_call(gcall*)
        /home/seurer/gcc/git/gcc-test/gcc/internal-fn.c:4087
0x1044e617 expand_call_stmt
        /home/seurer/gcc/git/gcc-test/gcc/cfgexpand.c:2749
0x1044e617 expand_gimple_stmt_1
        /home/seurer/gcc/git/gcc-test/gcc/cfgexpand.c:3844
0x1044fd5f expand_gimple_stmt
        /home/seurer/gcc/git/gcc-test/gcc/cfgexpand.c:4008
0x104577b3 expand_gimple_basic_block
        /home/seurer/gcc/git/gcc-test/gcc/cfgexpand.c:6045
0x1045a28b execute
        /home/seurer/gcc/git/gcc-test/gcc/cfgexpand.c:6729

commit d0a5e8e1a84bdd6ce915c3be65da8af2552cd49e
Author: Xionghu Luo <luoxhu@linux.ibm.com>
Date:   Sun Mar 21 21:14:02 2021 -0500

    rs6000: Convert the vector set variable idx to DImode [PR98914]

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[luoxhu at gcc dot gnu.org]

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

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[rguenth at gcc dot gnu.org]

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?

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[rguenth at gcc dot gnu.org]

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

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[jakub at gcc dot gnu.org]

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")))

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[luoxhu at gcc dot gnu.org]

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]);

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[segher at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[jakub at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[segher at gcc dot gnu.org]

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

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[dje at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[dje at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[jakub at gcc dot gnu.org]

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

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[luoxhu at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[luoxhu at gcc dot gnu.org]

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?

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[luoxhu at gcc dot gnu.org]

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

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[jakub at gcc dot gnu.org]

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 ...

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[luoxhu at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[jakub at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[segher at gcc dot gnu.org]

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 :-(

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[segher at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[luoxhu at gcc dot gnu.org]

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

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[cvs-commit at gcc dot gnu.org]

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.

[Bug target/99718] [11 regression] ICE in new test case gcc.target/powerpc/pr98914.c for 32 bits

[luoxhu at gcc dot gnu.org]

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.