On Thu, May 04, 2006 at 08:26:52PM +0300, Leehod Baruch wrote:
> Please, lets be more precise.
> All the problem you have listed here are problems that relates x86.
> There is no problem on PPC and as far as I know there is no problem
> on other platforms (at least no one complained about it).
> *ALL* the problems you have mentioned appears only on x86 and most of
> them are a result of changes that we tried to do in order to make this
> optimization work better for Intel.
> Just a reminder, we concluded that this optimization in it's current state
> is not relevant for Intel and therefore all these changes/bugs are
> irrelevant now.
> As far as the committed patch there is no problem except one
> _minor_ correction that will be submitted in the follow-up patch.
>
> > On Thu, May 04, 2006 at 03:25:22PM +0200, Mircea Namolaru wrote:
> >> The patches for SEE have been committed today.
> >>
> >> The minor style corrections requested by you in the
> >> final review approval will be in a follow-up patch
> >> to be submitted the next week.
> >>
> >
> > I didn't see you have addressed the issuses I raised in my previous
> > emails. When I used
> >
> > export BOOT_CFLAGS="-g -O2 -fsee" CXXFLAGS="-g -O2 -fsee" FCFLAGS="-g -O2
> > -fsee" GCJFLAGS="-g -O2 -fsee" SYSROOT_CFLAGS_FOR_TARGET="-g -O2 -fsee"
> > # ..../configure
> > # make BOOT_CFLAGS="-g -O2 -fsee" CXXFLAGS="-g -O2 -fsee" FCFLAGS="-g -O2
> > -fsee" GCJFLAGS="-g -O2 -fsee" SYSROOT_CFLAGS_FOR_TARGET="-g -O2 -fsee"
> >
> > to configure and build gcc on Linux/x86 and Linux/x86-64. They both
> This is relevant for x86.
# export BOOT_CFLAGS="-g -O2 -fsee" CXXFLAGS="-g -O2 -fsee" FCFLAGS="-g -O2
-fsee" GCJFLAGS="-g -O2 -fsee" SYSROOT_CFLAGS_FOR_TARGET="-g -O2 -fsee"
# ...../configure
# make BOOT_CFLAGS="-g -O2 -fsee" CXXFLAGS="-g -O2 -fsee" FCFLAGS="-g -O2
-fsee" GCJFLAGS="-g -O2 -fsee" SYSROOT_CFLAGS_FOR_TARGET="-g -O2 -fsee"
is applying SEE on gcc sources. Why is this only relevant for x86?
> > failed to bootstrap. There are at least 2 problems:
> >
> > 1. SEE uses NEXT_INSN/PREV_INSN to find adjacent insns. But with
> > -g, NEXT_INSN/PREV_INSN may point to a NOTE. So adjacent insns checks
> > with NEXT_INSN/PREV_INSN aren't sufficient.
> Only if we change the code to catch x86 patterns.
With -g, I may get
(note:HI 17 14 18 2
("/net/gnu-13/export/gnu/src/gcc-see/gcc/gcc/testsuite/gcc.c-torture/execute/20010224-1.c")
18)
(insn:HI 18 17 19 2
/net/gnu-13/export/gnu/src/gcc-see/gcc/gcc/testsuite/gcc.c-torture/execute/20010224-1.c:18
(set (reg/v:SI 70 [ j ])
(sign_extend:SI (subreg:HI (reg:SI 72 [ start ]) 0))) 118 {extendhisi2}
(insn_list:REG_DEP_TRUE 12 (nil))
(expr_list:REG_DEAD (reg:SI 72 [ start ])
(nil)))
(note:HI 19 18 20 2
("/net/gnu-13/export/gnu/src/gcc-see/gcc/gcc/testsuite/gcc.c-torture/execute/20010224-1.c")
19)
(insn:HI 20 19 22 2
/net/gnu-13/export/gnu/src/gcc-see/gcc/gcc/testsuite/gcc.c-torture/execute/20010224-1.c:19
(set (reg:DI 73 [ j ])
(sign_extend:DI (reg/v:SI 70 [ j ]))) 115 {extendsidi2_rex64}
(insn_list:REG_DEP_TRUE 18 (nil))
(nil))
Those notes may be added between insns because -g. NEXT_INSN/PREV_INSN
won't get you the adjacent insn in this case.
> > 2. Not all zero_extend patterns are available for x86/x86-64. For
> > example:
> >
> > (insn 137 0 0 (set (reg:SI 60 [ prephitmp.115 ])
> > (zero_extend:SI (subreg:QI (reg:SI 60 [ prephitmp.115 ]) 0)))
> > -1 (nil)
> > (nil))
> >
> > may not be used on x86/x86-64. i386.md has
> >
> > (define_expand "zero_extendqisi2"
> > [(parallel
> > [(set (match_operand:SI 0 "register_operand" "")
> > (zero_extend:SI (match_operand:QI 1 "nonimmediate_operand" "")))
> > (clobber (reg:CC FLAGS_REG))])]
> > ""
> > "")
> >
> > This is case for all extensions for i386. For x86-64, only
> > zero_extendsidi2 won't clobber CC.
> Again, for x86.
But SEE doesn't provide a way to deal with it.
> > 3. When the original insns were
> >
> > set (dest_extension_reg1) (sign_extend (source_extension_reg1))
> > set (dest_extension_reg2) (sign_extend (dest_extension_reg1))
> >
> > We created
> >
> > ref: set (dest_extension_reg1) (sign_extend (source_extension_reg1))
> > def_se: set (dest_extension_reg2) (sign_extend (dest_extension_reg1))
> >
> > and
> >
> > use_se: set (dest_extension_reg1) (sign_extend (dest_extension_reg1))
> > ref: set (dest_extension_reg2) (sign_extend (dest_extension_reg1))
> >
> > When def merge failed, def_se was deleted. Now use_se had a deleted
> > ref.
> >
> > Basically, SEE doesn't handle
> >
> > (set (reg/v:SI 70 [ j ])
> > (sign_extend:SI (subreg:HI (reg:SI 72 [ start ]) 0)))
> > (set (reg:DI 73 [ j ])
> > (sign_extend:DI (reg/v:SI 70 [ j ])))
> >
> > correctly.
> This is not true. SEE handles this. The problem that you saw are a result
> of Denis' change.
> >
> > 4. SEE also failed to handle
> >
> > set (dest_extension_reg1) (zero_extend (source_extension_reg1))
> > set (reg) (..dest_extension_reg1..)
> > set (dest_extension_reg2) (sign_extend (source_extension_reg1))
> >
> > (insn:HI 28 26 30 2 x.c:1201 (set (reg:DI 534 [ mode ])
> > (zero_extend:DI (reg/v:SI 264 [ mode ]))) 111
> > {zero_extendsidi2_rex64}
> > (insn_list:REG_DEP_TRUE 11 (nil))
> > (nil))
> >
> > (insn:HI 30 28 269 2 x.c:1201 (set (reg:QI 261 [ D.24257 ])
> > (mem/s/u:QI (plus:DI (reg:DI 534 [ mode ])
> > (symbol_ref:DI ("mode_class") [flags 0x40] <var_decl
> > 0x2a98a42630 mode_class>)) [0 mode_class S1 A8])) 55 {*movqi_1}
> > (insn_list:REG_DEP_TRUE 28 (nil))
> > (nil))
> >
> > (insn:HI 269 30 270 2 x.c:1273 (set (reg:DI 546)
> > (sign_extend:DI (reg/v:SI 264 [ mode ]))) 115
> > {extendsidi2_rex64} (nil)
> > (nil))
> This pattern is not relevant without Denis' change.
Those patterns may happen since SEE uses NEXT_INSN/PREV_INSN to check
the adjacent insn. When -g is used, SEE may see the note instead of
the real adjacent insn and reaches wrong conclusion. It may lead to
compiler crash or wrong code.
H.J.
