On 09/05/17 17:02, Wilco Dijkstra wrote:
> Bernd Edlinger wrote:
> 
>> Combine creates an invalid insn out of these two insns:
> 
> Yes it looks like a latent bug. We need to use arm_general_register_operand
> as arm_adddi3/subdi3 only allow integer registers. You don't need a new predicate
> s_register_operand_nv. Also I'd prefer something like arm_general_adddi_operand.
> 

Thanks, attached is a patch following your suggestion.

> +  "TARGET_32BIT && ((!TARGET_NEON && !TARGET_IWMMXT) || reload_completed)"
> 
> The split condition for adddi3 now looks more accurate indeed, although we could
> remove the !TARGET_NEON from the split condition as this is always true given
> arm_adddi3 uses "TARGET_32BIT && !TARGET_NEON".
> 

No, the split condition does not begin with "&& TARGET_32BIT...".
Therefore the split is enabled in TARGET_NEON after reload_completed.
And it is invoked from adddi3_neon for all alternatives without vfp
registers:

   switch (which_alternative)
     {
     case 0: /* fall through */
     case 3: return "vadd.i64\t%P0, %P1, %P2";
     case 1: return "#";
     case 2: return "#";
     case 4: return "#";
     case 5: return "#";
     case 6: return "#";



> Also there are more cases, a quick grep suggests *anddi_notdi_di has the same issue.
> 

Yes, that pattern can be cleaned up in a follow-up patch.
Note this splitter is invoked from bicdi3_neon as well.
However I think anddi_notdi_di should be safe as long as it is enabled
after reload_completed (which is probably a bug).


Bernd.

> Wilco
>