public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH, VAX] Correct ffs instruction constraint
@ 2017-06-20 20:06 coypu
  2017-06-29 15:47 ` coypu
  0 siblings, 1 reply; 6+ messages in thread
From: coypu @ 2017-06-20 20:06 UTC (permalink / raw)
  To: gcc-patches; +Cc: matt, m4j0rd0m0, paulkoning, jbglaw

VAX' FFS as variable-length bit field instruction uses a "base"
operand of type "vb" meaning "byte address".
"base" can be 32 bits (SI) and due to the definition of
ffssi2/__builtin_ffs() with the operand constraint "m", code can be
emitted which incorrectly implies a mode-dependent (= longword, for
the 32-bit operand) address.
File scsipi_base.c compiled with -Os for our VAX install kernel shows:

    ffs $0x0,$0x20,0x50(r11)[r0],r9

Apparently, 0x50(r11)[r0] as a longword address is assumed to be
evaluated in longword context by FFS, but the instruction expects a
byte address.

Our fix is to change the operand constraint from "m" to "Q", i. e.
"operand is a MEM that does not have a mode-dependent address", which
results in:

    moval 0x50(r11)[r0],r1
    ffs $0x0,$0x20,(r1),r9

MOVAL evaluates the source operand/address in longword context, so
effectively converts the word address to a byte address for FFS.

See NetBSD PR port-vax/51761 (http://gnats.netbsd.org/51761) and
discussion on port-vax mailing list
(http://mail-index.netbsd.org/port-vax/2017/01/06/msg002954.html).

Changlog:

2017-06-20  Maya Rashish  <coypu@sdf.org>

	* gcc/config/vax/builtins.md: Correct ffssi2_internal
	instruction constraint.


---
 gcc/config/vax/builtins.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/vax/builtins.md b/gcc/config/vax/builtins.md
index fb0f69acb..b78fb5616 100644
--- a/gcc/config/vax/builtins.md
+++ b/gcc/config/vax/builtins.md
@@ -41,7 +41,7 @@
 
 (define_insn "ffssi2_internal"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=rQ")
-	(ffs:SI (match_operand:SI 1 "general_operand" "nrmT")))
+	(ffs:SI (match_operand:SI 1 "general_operand" "nrQT")))
    (set (cc0) (match_dup 0))]
   ""
   "ffs $0,$32,%1,%0")
-- 
2.13.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, VAX] Correct ffs instruction constraint
  2017-06-20 20:06 [PATCH, VAX] Correct ffs instruction constraint coypu
@ 2017-06-29 15:47 ` coypu
  2017-06-29 19:44   ` Jeff Law
  0 siblings, 1 reply; 6+ messages in thread
From: coypu @ 2017-06-29 15:47 UTC (permalink / raw)
  To: gcc-patches; +Cc: matt

Ping.

On Tue, Jun 20, 2017 at 08:05:42PM +0000, coypu@sdf.org wrote:
> VAX' FFS as variable-length bit field instruction uses a "base"
> operand of type "vb" meaning "byte address".
> "base" can be 32 bits (SI) and due to the definition of
> ffssi2/__builtin_ffs() with the operand constraint "m", code can be
> emitted which incorrectly implies a mode-dependent (= longword, for
> the 32-bit operand) address.
> File scsipi_base.c compiled with -Os for our VAX install kernel shows:
> 
>     ffs $0x0,$0x20,0x50(r11)[r0],r9
> 
> Apparently, 0x50(r11)[r0] as a longword address is assumed to be
> evaluated in longword context by FFS, but the instruction expects a
> byte address.
> 
> Our fix is to change the operand constraint from "m" to "Q", i. e.
> "operand is a MEM that does not have a mode-dependent address", which
> results in:
> 
>     moval 0x50(r11)[r0],r1
>     ffs $0x0,$0x20,(r1),r9
> 
> MOVAL evaluates the source operand/address in longword context, so
> effectively converts the word address to a byte address for FFS.
> 
> See NetBSD PR port-vax/51761 (http://gnats.netbsd.org/51761) and
> discussion on port-vax mailing list
> (http://mail-index.netbsd.org/port-vax/2017/01/06/msg002954.html).
> 
> Changlog:
> 
> 2017-06-20  Maya Rashish  <coypu@sdf.org>
> 
> 	* gcc/config/vax/builtins.md: Correct ffssi2_internal
> 	instruction constraint.
> 
> 
> ---
>  gcc/config/vax/builtins.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/config/vax/builtins.md b/gcc/config/vax/builtins.md
> index fb0f69acb..b78fb5616 100644
> --- a/gcc/config/vax/builtins.md
> +++ b/gcc/config/vax/builtins.md
> @@ -41,7 +41,7 @@
>  
>  (define_insn "ffssi2_internal"
>    [(set (match_operand:SI 0 "nonimmediate_operand" "=rQ")
> -	(ffs:SI (match_operand:SI 1 "general_operand" "nrmT")))
> +	(ffs:SI (match_operand:SI 1 "general_operand" "nrQT")))
>     (set (cc0) (match_dup 0))]
>    ""
>    "ffs $0,$32,%1,%0")
> -- 
> 2.13.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, VAX] Correct ffs instruction constraint
  2017-06-29 15:47 ` coypu
@ 2017-06-29 19:44   ` Jeff Law
  2017-07-06 17:00     ` Felix Deichmann
  0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2017-06-29 19:44 UTC (permalink / raw)
  To: coypu, gcc-patches; +Cc: matt

On 06/29/2017 09:47 AM, coypu@sdf.org wrote:
> Ping.
> 
> On Tue, Jun 20, 2017 at 08:05:42PM +0000, coypu@sdf.org wrote:
>> VAX' FFS as variable-length bit field instruction uses a "base"
>> operand of type "vb" meaning "byte address".
>> "base" can be 32 bits (SI) and due to the definition of
>> ffssi2/__builtin_ffs() with the operand constraint "m", code can be
>> emitted which incorrectly implies a mode-dependent (= longword, for
>> the 32-bit operand) address.
>> File scsipi_base.c compiled with -Os for our VAX install kernel shows:
>>
>>     ffs $0x0,$0x20,0x50(r11)[r0],r9
>>
>> Apparently, 0x50(r11)[r0] as a longword address is assumed to be
>> evaluated in longword context by FFS, but the instruction expects a
>> byte address.
>>
>> Our fix is to change the operand constraint from "m" to "Q", i. e.
>> "operand is a MEM that does not have a mode-dependent address", which
>> results in:
>>
>>     moval 0x50(r11)[r0],r1
>>     ffs $0x0,$0x20,(r1),r9
>>
>> MOVAL evaluates the source operand/address in longword context, so
>> effectively converts the word address to a byte address for FFS.
>>
>> See NetBSD PR port-vax/51761 (http://gnats.netbsd.org/51761) and
>> discussion on port-vax mailing list
>> (http://mail-index.netbsd.org/port-vax/2017/01/06/msg002954.html).
>>
>> Changlog:
>>
>> 2017-06-20  Maya Rashish  <coypu@sdf.org>
>>
>> 	* gcc/config/vax/builtins.md: Correct ffssi2_internal
>> 	instruction constraint.
Thanks.  Installed.

Ideally we'd like to have a testcase for this in the regression suite.

If you could provide the .i file and options used which generated the
incorrect ffs instruction I can use the reduction tools with a cross
compiler to produce a nice simple test for the testsuite.

jeff

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, VAX] Correct ffs instruction constraint
  2017-06-29 19:44   ` Jeff Law
@ 2017-07-06 17:00     ` Felix Deichmann
  2017-07-06 18:53       ` Jeff Law
  0 siblings, 1 reply; 6+ messages in thread
From: Felix Deichmann @ 2017-07-06 17:00 UTC (permalink / raw)
  To: law; +Cc: coypu, gcc-patches, matt

Jeff,

Am 29.06.2017 schrieb Jeff Law <law@redhat.com>:
> Ideally we'd like to have a testcase for this in the regression suite.
> 
> If you could provide the .i file and options used which generated the
> incorrect ffs instruction I can use the reduction tools with a cross
> compiler to produce a nice simple test for the testsuite.

I put the corresponding .i file at:
http://www.netbsd.org/~flxd/scsipi_base.i.gz

See line 7638:
bit = __builtin_ffs(periph->periph_freetags[word]);

Command/Options used which generated the incorrect ffs instruction:

/nb8/obj/tooldir.NetBSD-7.0-amd64/bin/vax--netbsdelf-gcc -fno-pic
-ffreestanding -fno-zero-initialized-in-bss -Os -fno-strict-aliasing
-fno-common -std=gnu99 -Werror -Wall -Wno-main -Wno-format-zero-length
-Wpointer-arith -Wmissing-prototypes -Wstrict-prototypes
-Wold-style-definition -Wswitch -Wshadow -Wcast-qual -Wwrite-strings
-Wno-unreachable-code -Wno-pointer-sign -Wno-attributes
-Wno-sign-compare --sysroot=/nb8/obj/destdir.vax -D_VAX_INLINE_ -I.
-I/nb8/src/sys/../common/lib/libx86emu -I/nb8/src/sys/../common/include
-I/nb8/src/sys/arch -I/nb8/src/sys -nostdinc -D_KERNEL -D_KERNEL_OPT
-std=gnu99 -I/nb8/src/sys/lib/libkern/../../../common/lib/libc/quad
-I/nb8/src/sys/lib/libkern/../../../common/lib/libc/string
-I/nb8/src/sys/lib/libkern/../../../common/lib/libc/arch/vax/string -c
/nb8/src/sys/dev/scsipi/scsipi_base.c -o scsipi_base.o

Best regards,
Felix

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, VAX] Correct ffs instruction constraint
  2017-07-06 17:00     ` Felix Deichmann
@ 2017-07-06 18:53       ` Jeff Law
  2017-07-07 14:55         ` Felix Deichmann
  0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2017-07-06 18:53 UTC (permalink / raw)
  To: Felix Deichmann; +Cc: coypu, gcc-patches, matt

On 07/06/2017 10:59 AM, Felix Deichmann wrote:
> Jeff,
> 
> Am 29.06.2017 schrieb Jeff Law <law@redhat.com>:
>> Ideally we'd like to have a testcase for this in the regression suite.
>>
>> If you could provide the .i file and options used which generated the
>> incorrect ffs instruction I can use the reduction tools with a cross
>> compiler to produce a nice simple test for the testsuite.
> 
> I put the corresponding .i file at:
> http://www.netbsd.org/~flxd/scsipi_base.i.gz
> 
> See line 7638:
> bit = __builtin_ffs(periph->periph_freetags[word]);
> 
> Command/Options used which generated the incorrect ffs instruction:
> 
> /nb8/obj/tooldir.NetBSD-7.0-amd64/bin/vax--netbsdelf-gcc -fno-pic
> -ffreestanding -fno-zero-initialized-in-bss -Os -fno-strict-aliasing
> -fno-common -std=gnu99 -Werror -Wall -Wno-main -Wno-format-zero-length
> -Wpointer-arith -Wmissing-prototypes -Wstrict-prototypes
> -Wold-style-definition -Wswitch -Wshadow -Wcast-qual -Wwrite-strings
> -Wno-unreachable-code -Wno-pointer-sign -Wno-attributes
> -Wno-sign-compare --sysroot=/nb8/obj/destdir.vax -D_VAX_INLINE_ -I.
> -I/nb8/src/sys/../common/lib/libx86emu -I/nb8/src/sys/../common/include
> -I/nb8/src/sys/arch -I/nb8/src/sys -nostdinc -D_KERNEL -D_KERNEL_OPT
> -std=gnu99 -I/nb8/src/sys/lib/libkern/../../../common/lib/libc/quad
> -I/nb8/src/sys/lib/libkern/../../../common/lib/libc/string
> -I/nb8/src/sys/lib/libkern/../../../common/lib/libc/arch/vax/string -c
> /nb8/src/sys/dev/scsipi/scsipi_base.c -o scsipi_base.o
Hmm, unfortunately I consistently get a call to into libgcc for the
__builtin_ffs code rather than an ffs instruction.  That's with a
gcc-4.8.3 as well as with trunk compiler.

Can you include "-v" output from compiling scsipi_base?

Thanks.
jeff

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH, VAX] Correct ffs instruction constraint
  2017-07-06 18:53       ` Jeff Law
@ 2017-07-07 14:55         ` Felix Deichmann
  0 siblings, 0 replies; 6+ messages in thread
From: Felix Deichmann @ 2017-07-07 14:55 UTC (permalink / raw)
  To: Jeff Law; +Cc: coypu, gcc-patches, matt

Am 06.07.2017 um 20:53 schrieb Jeff Law:
> Hmm, unfortunately I consistently get a call to into libgcc for the
> __builtin_ffs code rather than an ffs instruction.  That's with a
> gcc-4.8.3 as well as with trunk compiler.
> 
> Can you include "-v" output from compiling scsipi_base?

Hope this is right/enough:

Using built-in specs.
COLLECT_GCC=/nb8/obj/tooldir.NetBSD-7.0-amd64/bin/vax--netbsdelf-gcc
Target: vax--netbsdelf
Configured with:
/nb8/src/tools/gcc/../../external/gpl3/gcc/dist/configure
--target=vax--netbsdelf --enable-long-long --enable-threads
--with-bugurl=http://www.NetBSD.org/Misc/send-pr.html
--with-pkgversion='NetBSD nb1 20160606' --with-system-zlib
--enable-__cxa_atexit --enable-libstdcxx-time=rt
--enable-libstdcxx-threads --with-diagnostics-color=auto-if-env
--with-sysroot=/nb8/obj/destdir.vax
--with-mpc=/nb8/obj/tooldir.NetBSD-7.0-amd64
--with-mpfr=/nb8/obj/tooldir.NetBSD-7.0-amd64
--with-gmp=/nb8/obj/tooldir.NetBSD-7.0-amd64 --disable-nls
--disable-multilib --program-transform-name=s,^,vax--netbsdelf-,
--enable-languages='c c++ objc' --prefix=/nb8/obj/tooldir.NetBSD-7.0-amd64
Thread model: posix
gcc version 5.4.0 (NetBSD nb1 20160606)
COLLECT_GCC_OPTIONS='-v' '-fno-pic' '-ffreestanding'
'-fno-zero-initialized-in-bss' '-Os' '-fno-strict-aliasing'
'-fno-common' '-std=gnu99' '-Werror' '-Wall' '-Wno-main'
'-Wno-format-zero-length' '-Wpointer-arith' '-Wmissing-prototypes'
'-Wstrict-prototypes' '-Wold-style-definition' '-Wswitch' '-Wshadow'
'-Wcast-qual' '-Wwrite-strings' '-Wno-pointer-sign' '-Wno-attributes'
'-Wno-sign-compare' '-D' '_VAX_INLINE_' '-I' '.' '-I'
'/nb8/src/sys/../common/lib/libx86emu' '-I'
'/nb8/src/sys/../common/include' '-I' '/nb8/src/sys/arch' '-I'
'/nb8/src/sys' '-nostdinc' '-D' '_KERNEL' '-D' '_KERNEL_OPT'
'-std=gnu99' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/quad' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/string' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/arch/vax/string' '-c'
'-o' 'scsipi_base.o'
 /nb8/obj/tooldir.NetBSD-7.0-amd64/libexec/gcc/vax--netbsdelf/5.4.0/cc1
-quiet -nostdinc -v -I . -I /nb8/src/sys/../common/lib/libx86emu -I
/nb8/src/sys/../common/include -I /nb8/src/sys/arch -I /nb8/src/sys -I
/nb8/src/sys/lib/libkern/../../../common/lib/libc/quad -I
/nb8/src/sys/lib/libkern/../../../common/lib/libc/string -I
/nb8/src/sys/lib/libkern/../../../common/lib/libc/arch/vax/string
-isysroot /nb8/obj/destdir.vax -D _VAX_INLINE_ -D _KERNEL -D _KERNEL_OPT
/nb8/src/sys/dev/scsipi/scsipi_base.c -quiet -dumpbase scsipi_base.c
-auxbase-strip scsipi_base.o -Os -Werror -Wall -Wno-main
-Wno-format-zero-length -Wpointer-arith -Wmissing-prototypes
-Wstrict-prototypes -Wold-style-definition -Wswitch -Wshadow -Wcast-qual
-Wwrite-strings -Wno-pointer-sign -Wno-attributes -Wno-sign-compare
-std=gnu99 -std=gnu99 -version -fno-pic -ffreestanding
-fno-zero-initialized-in-bss -fno-strict-aliasing -fno-common -o
/var/tmp//ccy92Bnl.s
GNU C99 (NetBSD nb1 20160606) version 5.4.0 (vax--netbsdelf)
	compiled by GNU C version 4.8.4, GMP version 5.1.3, MPFR version 3.1.2,
MPC version 1.0.1
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
#include "..." search starts here:
#include <...> search starts here:
 .
 /nb8/src/sys/../common/lib/libx86emu
 /nb8/src/sys/../common/include
 /nb8/src/sys/arch
 /nb8/src/sys
 /nb8/src/sys/lib/libkern/../../../common/lib/libc/quad
 /nb8/src/sys/lib/libkern/../../../common/lib/libc/string
 /nb8/src/sys/lib/libkern/../../../common/lib/libc/arch/vax/string
End of search list.
GNU C99 (NetBSD nb1 20160606) version 5.4.0 (vax--netbsdelf)
	compiled by GNU C version 4.8.4, GMP version 5.1.3, MPFR version 3.1.2,
MPC version 1.0.1
GGC heuristics: --param ggc-min-expand=100 --param ggc-min-heapsize=131072
Compiler executable checksum: 530755d1b0ada9bef015017ee74733db
COLLECT_GCC_OPTIONS='-v' '-fno-pic' '-ffreestanding'
'-fno-zero-initialized-in-bss' '-Os' '-fno-strict-aliasing'
'-fno-common' '-std=gnu99' '-Werror' '-Wall' '-Wno-main'
'-Wno-format-zero-length' '-Wpointer-arith' '-Wmissing-prototypes'
'-Wstrict-prototypes' '-Wold-style-definition' '-Wswitch' '-Wshadow'
'-Wcast-qual' '-Wwrite-strings' '-Wno-pointer-sign' '-Wno-attributes'
'-Wno-sign-compare' '-D' '_VAX_INLINE_' '-I' '.' '-I'
'/nb8/src/sys/../common/lib/libx86emu' '-I'
'/nb8/src/sys/../common/include' '-I' '/nb8/src/sys/arch' '-I'
'/nb8/src/sys' '-nostdinc' '-D' '_KERNEL' '-D' '_KERNEL_OPT'
'-std=gnu99' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/quad' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/string' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/arch/vax/string' '-c'
'-o' 'scsipi_base.o'
 /nb8/obj/tooldir.NetBSD-7.0-amd64/lib/gcc/vax--netbsdelf/5.4.0/../../../../vax--netbsdelf/bin/as -v -I . -I /nb8/src/sys/../common/lib/libx86emu -I /nb8/src/sys/../common/include -I /nb8/src/sys/arch -I /nb8/src/sys -I /nb8/src/sys/lib/libkern/../../../common/lib/libc/quad -I /nb8/src/sys/lib/libkern/../../../common/lib/libc/string -I /nb8/src/sys/lib/libkern/../../../common/lib/libc/arch/vax/string -o scsipi_base.o /var/tmp//ccy92Bnl.s
GNU assembler version 2.27 (vax--netbsdelf) using BFD version (NetBSD
Binutils nb1) 2.27
COMPILER_PATH=/nb8/obj/tooldir.NetBSD-7.0-amd64/libexec/gcc/vax--netbsdelf/5.4.0/:/nb8/obj/tooldir.NetBSD-7.0-amd64/libexec/gcc/vax--netbsdelf/5.4.0/:/nb8/obj/tooldir.NetBSD-7.0-amd64/libexec/gcc/vax--netbsdelf/:/nb8/obj/tooldir.NetBSD-7.0-amd64/lib/gcc/vax--netbsdelf/5.4.0/:/nb8/obj/tooldir.NetBSD-7.0-amd64/lib/gcc/vax--netbsdelf/:/nb8/obj/tooldir.NetBSD-7.0-amd64/lib/gcc/vax--netbsdelf/5.4.0/../../../../vax--netbsdelf/bin/
LIBRARY_PATH=/nb8/obj/tooldir.NetBSD-7.0-amd64/lib/gcc/vax--netbsdelf/5.4.0/:/nb8/obj/tooldir.NetBSD-7.0-amd64/lib/gcc/vax--netbsdelf/5.4.0/../../../../vax--netbsdelf/lib/:/nb8/obj/destdir.vax/usr/lib/
COLLECT_GCC_OPTIONS='-v' '-fno-pic' '-ffreestanding'
'-fno-zero-initialized-in-bss' '-Os' '-fno-strict-aliasing'
'-fno-common' '-std=gnu99' '-Werror' '-Wall' '-Wno-main'
'-Wno-format-zero-length' '-Wpointer-arith' '-Wmissing-prototypes'
'-Wstrict-prototypes' '-Wold-style-definition' '-Wswitch' '-Wshadow'
'-Wcast-qual' '-Wwrite-strings' '-Wno-pointer-sign' '-Wno-attributes'
'-Wno-sign-compare' '-D' '_VAX_INLINE_' '-I' '.' '-I'
'/nb8/src/sys/../common/lib/libx86emu' '-I'
'/nb8/src/sys/../common/include' '-I' '/nb8/src/sys/arch' '-I'
'/nb8/src/sys' '-nostdinc' '-D' '_KERNEL' '-D' '_KERNEL_OPT'
'-std=gnu99' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/quad' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/string' '-I'
'/nb8/src/sys/lib/libkern/../../../common/lib/libc/arch/vax/string' '-c'
'-o' 'scsipi_base.o'

Cheers,
Felix

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-07-07 14:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-20 20:06 [PATCH, VAX] Correct ffs instruction constraint coypu
2017-06-29 15:47 ` coypu
2017-06-29 19:44   ` Jeff Law
2017-07-06 17:00     ` Felix Deichmann
2017-07-06 18:53       ` Jeff Law
2017-07-07 14:55         ` Felix Deichmann

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