On 5 September 2017 at 20:20, Christophe Lyon
<christophe.l...@linaro.org> wrote:
> 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.
>
I can confirm the last patch does fix the regression I reported, and causes
no other regression. (The previous previous of the fix, worked, too).
Thanks for the prompt fix.
Christophe
> Christophe
>
>> Kyrill
>>
>>
>>> Bernd.
>>>
>>>> Wilco
>>>>
>>
