Bug 83789 - __builtin_altivec_lvx fails for powerpc for altivec-4.c
Summary: __builtin_altivec_lvx fails for powerpc for altivec-4.c
Status: CLOSED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: 7.4
Assignee: Peter Bergner
URL: https://gcc.gnu.org/ml/gcc-patches/20...
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-11 10:23 UTC by Kaushikp
Modified: 2018-03-26 10:27 UTC (History)
5 users (show)

See Also:
Host:
Target: powerpc
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Proposed patch to merge altivec patterns. (1.87 KB, patch)
2018-02-14 19:22 UTC, Peter Bergner
Details | Diff
Updated patch without debug cruft. No other changes. (1.52 KB, patch)
2018-02-14 22:22 UTC, Peter Bergner
Details | Diff
Updated patch (3.25 KB, patch)
2018-03-10 04:16 UTC, Peter Bergner
Details | Diff
Yet another updated patch (3.25 KB, patch)
2018-03-10 16:29 UTC, Peter Bergner
Details | Diff
Version 3 of the patch (3.15 KB, patch)
2018-03-11 03:20 UTC, Peter Bergner
Details | Diff
Backport of trunk patch to GCC 7 (1.84 KB, patch)
2018-03-21 18:58 UTC, Peter Bergner
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kaushikp 2018-01-11 10:23:48 UTC
Regression testcase gcc.target/powerpc/altivec-4.c fails for powerpc gcc-7 onwards.

$powerpc-linux-gcc -m32 -mhard-float -maltivec altivec-4.c -c
min.c: In function 'b':
min.c:16:7: internal compiler error: Segmentation fault
   i = __builtin_altivec_lvx (int1, pi);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
0x85d8690 crash_signal
        /home/kaushikp/toolchain/powerpc/linux_build_71/../src/gcc-7.1.0/gcc/toplev.c:337
0x832f525 store_expr_with_bounds(tree_node*, rtx_def*, int, bool, bool, tree_node*)
        /home/kaushikp/toolchain/powerpc/linux_build_71/../src/gcc-7.1.0/gcc/expr.c:5575
0x8330c91 expand_assignment(tree_node*, tree_node*, bool)
        /home/kaushikp/toolchain/powerpc/linux_build_71/../src/gcc-7.1.0/gcc/expr.c:5321
0x823bb98 expand_call_stmt
        /home/kaushikp/toolchain/powerpc/linux_build_71/../src/gcc-7.1.0/gcc/cfgexpand.c:2656
0x823bb98 expand_gimple_stmt_1
        /home/kaushikp/toolchain/powerpc/linux_build_71/../src/gcc-7.1.0/gcc/cfgexpand.c:3571
0x823bb98 expand_gimple_stmt
        /home/kaushikp/toolchain/powerpc/linux_build_71/../src/gcc-7.1.0/gcc/cfgexpand.c:3737
0x823cbd0 expand_gimple_basic_block
        /home/kaushikp/toolchain/powerpc/linux_build_71/../src/gcc-7.1.0/gcc/cfgexpand.c:5744
0x8241e36 execute
        /home/kaushikp/toolchain/powerpc/linux_build_71/../src/gcc-7.1.0/gcc/cfgexpand.c:6357
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions

The builtin generates the 'CODE_FOR_altivec_lvx_v4si_2op' instruction which 
returns CODE_FOR_nothing resulting in Segmentation fault.
The pattern 'CODE_FOR_altivec_lvx_v4si_2op' was introduced by the following patch I believe,
https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01110.html

The pattern tests for TARGET_64BIT and the testcase passes for powerpc64,
however fails for 32-bit targets. This passed in GCC-6 and earlier branches.

Should we modify the testcase so it runs only for 64-bit targets, 
or should this builtin work for 32-bit targets as well?
Comment 1 Kaushikp 2018-01-16 01:41:47 UTC
Just some additional info on this issue, 
The powerpc-elf-gcc did not have any issues compiling this testcase with identical options on gcc-7.2.0; 
powerpc-linux-gcc generated segmentation fault; 
powerpc64-linux-gcc passed without errors or warnings.
Comment 2 Segher Boessenkool 2018-01-16 11:44:31 UTC
(In reply to Kaushikp from comment #0)

> Should we modify the testcase so it runs only for 64-bit targets, 
> or should this builtin work for 32-bit targets as well?

It shouldn't ICE for 32-bit either, whether the testcase really applies
to 32-bit or not (I haven't looked).
Comment 3 Segher Boessenkool 2018-01-16 20:27:51 UTC
Does not reproduce with powerpc64-linux-gcc -m32 (not on trunk and
not on current 7).
Comment 4 Segher Boessenkool 2018-02-07 18:14:22 UTC
Kaushik: is this fixed with r256762?
Comment 5 Kaushikp 2018-02-08 09:07:05 UTC
>> Kaushik: is this fixed with r256762?
No. The testcase still fails with internal compiler error: Segmentation fault.
This revision (r256762) dated 16th Jan 2018 still calls 'CODE_FOR_altivec_lvx_v4si_2op' insn
for ALTIVEC_BUILTIN_LVX. This is only defined for 64-bit target, hence it fails.

The latest revision r257477 dated today, 8th Feb 2018 also suffers from the same issue.
Comment 6 Peter Bergner 2018-02-14 04:15:17 UTC
Segher, I can have a look at this if you aren't already.  I can see how of the builtin only uses the altivec_lxv_<mode>_2op patterm, how it would fail on a -m32 compile, since that pattern is disabled and you're supposed to use altivec_lxv_<mode>_2op_si.

That said, why do we have two patterns at all?  It seems like they should be able to be combined fairly easily using the P iterator:

; The next two patterns embody what lvx should usually look like.
(define_insn "altivec_lvx_<mode>_2op"
  [(set (match_operand:VM2 0 "register_operand" "=v")
        (mem:VM2 (and:DI (plus:DI (match_operand:DI 1 "register_operand" "b")
                                  (match_operand:DI 2 "register_operand" "r"))
                         (const_int -16))))]
  "TARGET_ALTIVEC && TARGET_64BIT"
  "lvx %0,%1,%2"
  [(set_attr "type" "vecload")])

; 32-bit versions of the above.
(define_insn "altivec_lvx_<mode>_2op_si"
  [(set (match_operand:VM2 0 "register_operand" "=v")
        (mem:VM2 (and:SI (plus:SI (match_operand:SI 1 "register_operand" "b")
                                  (match_operand:SI 2 "register_operand" "r"))
                         (const_int -16))))]
  "TARGET_ALTIVEC && TARGET_32BIT"
  "lvx %0,%1,%2"
  [(set_attr "type" "vecload")])
Comment 7 Segher Boessenkool 2018-02-14 11:22:12 UTC
See https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00031.html -- I think Kaushik
is still looking at this?
Comment 8 Peter Bergner 2018-02-14 13:57:06 UTC
(In reply to Segher Boessenkool from comment #7)
> See https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00031.html -- I think
> Kaushik
> is still looking at this?

Ah, ok.  I do agree with your comment to use the *_2op_si pattern.

Do you agree we could combine these patterns?  If so, I could do that as a followon cleanup patch.
Comment 9 Peter Bergner 2018-02-14 19:22:48 UTC
Created attachment 43417 [details]
Proposed patch to merge altivec patterns.

Hi Kaushik,

Can you see if the following unetsted patch fixes your ICE?  In the end, it's cleaner to just combine the two patterns.
Comment 10 Peter Bergner 2018-02-14 22:22:39 UTC
Created attachment 43419 [details]
Updated patch without debug cruft.  No other changes.

Oops, I didn't mean to leave in stuff to make my make check less noisy.  Here's an updated patch.
Comment 11 Peter Bergner 2018-02-14 23:05:06 UTC
Ah, testing my patch, we're getting duplicate named patterns.  I'll have to think about this.
Comment 12 Kaushikp 2018-02-15 12:14:04 UTC
>> we're getting duplicate named patterns.
Yes, it does generate multiple patters for the concerned built-ins causing toolchain build failure.
I think it's trying to force the same pattern for 32-bit which exist for 64-bit
causing redefinition error.
Comment 13 Peter Bergner 2018-03-09 18:27:02 UTC
Ok, I have a patch that eliminates the duplicate patterns and greatly simplifies the code that calls these patterns.  I'm testing it now.
Comment 14 Peter Bergner 2018-03-10 04:16:00 UTC
Created attachment 43611 [details]
Updated patch

Here's the patch I'm testing now that removes the duplicate patterns and simplifies the callers of the patterns.

Kaushik, can you test that this fixes the ICE you're seeing, since Segher and I cannot recreate the ICE you're seeing?
Comment 15 Peter Bergner 2018-03-10 16:29:57 UTC
Created attachment 43617 [details]
Yet another updated patch

Sorry, the previous patch had a typo that only seemed to show up on my LE testing.  Please test this patch to see if it fixes your ICE.
Comment 16 kaushikp 2018-03-10 19:07:37 UTC
Thanks for the patch Peter.
gcc builds without errors on trunk with this patch.

I will let you know how my tests go.
Comment 17 Peter Bergner 2018-03-11 03:20:59 UTC
Created attachment 43621 [details]
Version 3 of the patch

Final version of the patch that bootstraps and regtests with no regressions on both BE and LE.  The previous patch had some testsuite regressions on LE.
Comment 18 Peter Bergner 2018-03-20 17:25:41 UTC
Author: bergner
Date: Tue Mar 20 17:25:09 2018
New Revision: 258688

URL: https://gcc.gnu.org/viewcvs?rev=258688&root=gcc&view=rev
Log:
	PR target/83789
	* config/rs6000/altivec.md (altivec_lvx_<mode>_2op): Delete define_insn.
	(altivec_lvx_<mode>_1op): Likewise.
	(altivec_stvx_<mode>_2op): Likewise.
	(altivec_stvx_<mode>_1op): Likewise.
	(altivec_lvx_<VM2:mode>): New define_expand.
	(altivec_stvx_<VM2:mode>): Likewise.
	(altivec_lvx_<VM2:mode>_2op_<P:mptrsize>): New define_insn.
	(altivec_lvx_<VM2:mode>_1op_<P:mptrsize>): Likewise.
	(altivec_stvx_<VM2:mode>_2op_<P:mptrsize>): Likewise.
	(altivec_stvx_<VM2:mode>_1op_<P:mptrsize>): Likewise.
	* config/rs6000/rs6000-p8swap.c (rs6000_gen_stvx): Use new expanders.
	(rs6000_gen_lvx): Likewise.
	* config/rs6000/rs6000.c (altivec_expand_lv_builtin): Likewise.
	(altivec_expand_stv_builtin): Likewise.
	(altivec_expand_builtin): Likewise.
	* config/rs6000/vector.md: Likewise.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/rs6000/altivec.md
    trunk/gcc/config/rs6000/rs6000-p8swap.c
    trunk/gcc/config/rs6000/rs6000.c
    trunk/gcc/config/rs6000/vector.md
Comment 19 Peter Bergner 2018-03-20 17:29:25 UTC
Fixed on trunk.
Comment 20 Peter Bergner 2018-03-20 17:31:58 UTC
Kaushik, remind me, you're seeing the same ICE in GCC 7 as well, so we need a backport of the patch committed to trunk?
Comment 21 Kaushik.Phatak 2018-03-21 04:27:47 UTC
Hi Peter,
Thanks for your work on this.

>> Kaushik, remind me, you're seeing the same ICE in GCC 7 as well
Yes, this does fail in gcc-7.x.

>> so we need a backport of the patch committed to trunk?
If a backport to GCC 7 is feasible, then that would be very much appreciated.
I tried patching parts of the file as-is, but it fails while patching the hunk to the file, rs6000-p8swap.c
This file (rs6000-p8swap.c) does not exist in gcc-7.x and was introduced in 8, I believe.

Thanks & Best Regards,
Kaushik M. Phatak

-----Original Message-----
From: bergner at gcc dot gnu.org [mailto:gcc-bugzilla@gcc.gnu.org] 
Sent: Tuesday, March 20, 2018 11:02 PM
To: Kaushik Phatak <Kaushik.Phatak@kpit.com>
Subject: [Bug target/83789] __builtin_altivec_lvx fails for powerpc for altivec-4.c

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

--- Comment #20 from Peter Bergner <bergner at gcc dot gnu.org> --- Kaushik, remind me, you're seeing the same ICE in GCC 7 as well, so we need a backport of the patch committed to trunk?

--
You are receiving this mail because:
You are on the CC list for the bug.
You reported the bug.
Comment 22 Peter Bergner 2018-03-21 14:41:12 UTC
(In reply to Kaushik.Phatak from comment #21)
> >> Kaushik, remind me, you're seeing the same ICE in GCC 7 as well
> Yes, this does fail in gcc-7.x.

Ok, then I'll work on backporting the patch.  Thanks.
Comment 23 Peter Bergner 2018-03-21 18:58:19 UTC
Created attachment 43728 [details]
Backport of trunk patch to GCC 7

Kaushik, can you verify the attached backported patch fixes the ICE on GCC 7?

This regtested fine on BE for me with no regressions.  My LE bootstrap/regtest is still running.
Comment 24 Peter Bergner 2018-03-21 21:25:45 UTC
(In reply to Peter Bergner from comment #23)
> This regtested fine on BE for me with no regressions.  My LE
> bootstrap/regtest is still running.

My LE bootstrap and regtesting were clean too.  Just waiting on Kaushik to verify the patch fixes the ICE on GCC 7.
Comment 25 Peter Bergner 2018-03-23 17:49:30 UTC
Author: bergner
Date: Fri Mar 23 17:48:58 2018
New Revision: 258819

URL: https://gcc.gnu.org/viewcvs?rev=258819&root=gcc&view=rev
Log:
	Backport from mainline
	2018-03-20  Peter Bergner  <bergner@vnet.ibm.com>

	PR target/83789
	* config/rs6000/altivec.md (altivec_lvx_<mode>_2op): Delete define_insn.
	(altivec_lvx_<mode>_1op): Likewise.
	(altivec_stvx_<mode>_2op): Likewise.
	(altivec_stvx_<mode>_1op): Likewise.
	(altivec_lvx_<VM2:mode>): New define_expand.
	(altivec_stvx_<VM2:mode>): Likewise.
	(altivec_lvx_<VM2:mode>_2op_<P:mptrsize>): New define_insn.
	(altivec_lvx_<VM2:mode>_1op_<P:mptrsize>): Likewise.
	(altivec_stvx_<VM2:mode>_2op_<P:mptrsize>): Likewise.
	(altivec_stvx_<VM2:mode>_1op_<P:mptrsize>): Likewise.
	* config/rs6000/rs6000.c (altivec_expand_lv_builtin): Likewise.
	(altivec_expand_stv_builtin): Likewise.
	(altivec_expand_builtin): Likewise.
	* config/rs6000/vector.md: Likewise.

Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/config/rs6000/altivec.md
    branches/gcc-7-branch/gcc/config/rs6000/rs6000.c
    branches/gcc-7-branch/gcc/config/rs6000/vector.md
Comment 26 Peter Bergner 2018-03-23 17:51:19 UTC
Backport to GCC 7 committed.  Closing as fixed.
Comment 27 kaushikp 2018-03-26 10:27:26 UTC
I have verified the backported patch to GCC-7 and it fixes the issues
I had observed earlier.

Thanks again Peter for this!