On 5 September 2017 at 19:53, Kyrill Tkachov <kyrylo.tkac...@foss.arm.com> wrote: > > On 05/09/17 18:48, Bernd Edlinger wrote: >> >> 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). >> > > Thanks, that's what I had in mind in my other reply. > This is ok if testing comes back ok. >
I've submitted the patch for testing, I'll let you know about the results. Christophe > Kyrill > > >> Bernd. >> >>> Wilco >>> >