Re: [PATCH] Fix PR33642, unrecognizable insn for -frtl-abstract-sequences

[Jim Wilson <wilson at tuliptree dot org>]

Gábor Lóki wrote:
> Bootstrapped on i686-linux, x86_64-linux.
> Regtested on i686-linux, x86_64-linux, arm-eabi, mips-elf, powerpc-elf, 
> sh-elf.

rtl-factoring aka seqabstr isn't enabled by default, and there are only 
2 testcases in the testsuite, so this isn't a very good test in general. 
 It would be a better test if you bootstrapped with seqabstr defaulted 
to on; or tried running the entire testsuite with seqabstr defaulted to on.

This mostly looks like an improvement.  It attacks the same problem I 
tried to fix for PR 35785 in a more general way.

One thing I notice is that you still have code checking 
REGNO_OK_FOR_INDIRECT_JUMP_P which should not be necessary now that you 
have the code to set ijmp_class and ijmp_reg.  And since this is an 
unused and undocumented macro we really either need to get rid of it or 
fix it (i.e. document it).

I tried building this for an ia64-linux cross compiler and noticed that 
I get a error linking cc1.
> libbackend.a(rtl-factoring.o): In function `get_default_length':
> /home/wilson/GCC/X-ia64-linux/gcc/../../gcc/gcc/rtl-factoring.c:329: undefined reference to `insn_default_length'

insn_default_length is only defined for targets that define a length 
attribute.  The IA-64 port does not.  Since this breaks building of all 
ia64 targets, I can't approve it as is.

I hacked in an insn_default_length function that returns 1 always and 
tried again.  It seems to work in this case, "work" meaning that it 
didn't ICE.  I didn't check whether correct code was emitted.  A proper 
fix here probably involves checking whether HAVE_ATTR_length is defined.

I noticed comments from Janis in PR 33642 pointing out that the patch 
makes this worse for rs6000 since we now get silent miscompilation 
instead of an ICE.  However, this isn't really a problem with this 
patch.  This is a problem with the optimization pass in general.  This 
patch is just exposing underlying existing problems with this 
optimization pass.  This needs a lot more work before it will usable for 
all targets.  So we can either accept that this optimization pass is 
currently unsafe, and that some patches may temporarily make things 
worse, or we can remove it until it is fixed and readd it later.  The 
second choice doesn't seem quite fair to you.  The first choice isn't 
fair to others.

As a temporary fix, one thing we could perhaps do is add warning 
messages for anyone using the option, for instance in process_options() 
in toplev.c.  That way people won't be expecting it to work if they try it.

Another thing we could do is modify the docs for the 
-frtl-abstract-sequences option to document that this option is known to 
be unsafe.  By the way, the docs for this option is in the middle of a 
table for options that control FP semantics.  This is definitely 
misplaced, and another thing that needs to be fixed.

I know that we have done things like this before for other options, but 
I don't recall the details.

Finally, one last comment, I don't like the fact that this optimization 
pass has 3 different names.  The file is rtl-factoring.c, the option 
name is -frtl-abstract-sequences, and in the dump files it is seqabstr. 
   I find this confusing.  I'm not asking you to fix this though, I'm 
just being grumpy.

Jim