Re: PATCH RFC: Fix ARM bug by splitting up iwmmxt_movsi_insn

[Richard Earnshaw <rearnsha at arm dot com>]

> This simple C++ program breaks the mainline C++ compiler when compiled
> with
>     -mcpu=iwmmxt -O2
> 
> inline const int&
> min(const int& __a, const int& __b)
> { if (__b < __a) return __b; return __a; }
> int foo (const char* f, const char* e, int max)
> { return min(max, e - f); }
> 
> The result is a crash with "could not split insn".
> 
> This happens because the ifcompare_plus_move insn requires a split,
> but the condition for the corresponding define_split has
> !TARGET_IWMMXT, with this comment:
>     ;; Note we have to suppress this split for the iwmmxt because it
>     ;; creates a conditional movsi and the iwmmxt_movsi_insn pattern
>     ;; is not predicable.  This sucks.
> 
> I was unable to find a simple C test case, although I didn't look too
> hard.  Note that even minor changes to the C++ test case cause it to
> compile successfully.
> 
> The simple way out of this is to change ifcompare_plus_move and
> friends to have a condition of !TARGET_IWMMXT.
> 
> However, it seems to me that the basic issue here may be wrong.
> Currently arm_movsi_insn has a condition of !TARGET_IWMMXT, and on the
> IWMMXT all moves are handled by iwmmxt_movsi_insn, which can not be
> predicable because (according to the comments) the alternative to
> store a IWMMXT general register to memory is not predicable.
> 
> But perhaps moves for ordinary registers can not continue to be
> handled by arm_movsi_insn, which is predicable, while moves for IWMMXT
> registers are handled by iwmmxt_movsi_insn.
> 
> That leads to the following patch.  This patch does not introduce any
> new testsuite compilation failures.  However, most ports don't seem to
> work this way.  In particular, it's possible that with the original
> code, in some cases, reload will sometimes choose a register in
> IWMMXT_GR_REGS, whereas with the patched code, it no longer will.  I'm
> not sure, and I'd like to hear any feedback which others may have.
> 
> (This patch includes a patch to arm_regno_class() which is really a
> simple bug fix.)

I'm concerned that the overall approach is wrong here.  In general you 
can't have two patterns in the MD that match the same pattern.  The 
compiler will just select the first and ignore the second.  Before 
register allocation nothing will be using wmmx registers explicitly 
(unless it's already matching a hard reg) so all the compiler will see 
when selecting register classes is those classes available for a non wmmx 
processor.

I think perhaps the best way to handle this is to force IF conversion to 
fail by defining a macro that will force the conditionalization to fail 
(for example, to make IFCVT_MODIFY_INSN return a NULL pattern for the 
problem cases).  Another alternative is to provide the predicated version 
of the move insn explicitly, and then to ensure that that version rejects 
the impossible cases (the predicable attribute simply means that the code 
generator will generate the predicated version of the pattern directly 
from the non-predicated version).

R.