[PATCH] D16291: AArch64: Implement missed conditional compare sequences.

[Marcello Maggioni via llvm-commits]

Hi Balaram,

The problem here is that the type (i64) is legal in general, but not for the OR operation, that’s why we have custom lowering for OR and i64.
Adding that check in this case I don’t think it would help :-/

Marcello
> On 4 Feb 2016, at 08:59, Balaram Makam <bmakam at codeaurora.org> wrote:
> 
> Hi Marcello,
> 
> Without making it LegalOrCustom it will break these lit-tests:
> CodeGen/AArch64/arm64-abi-varargs.ll
> CodeGen/AArch64/arm64-aapcs.ll
> 
> Would it help if we add a check to let legalize expand this if it isn't legal type yet during cutom lowering?
> 
> Adding something like in tryLowerToAArch64Cmp:
>  if (!DAG.getTargetLoweringInfo().isTypeLegal(VT))
>    return SDValue();
> 
> Thanks,
> Balaram
> -----Original Message-----
> From: mmaggioni at apple.com [mailto:mmaggioni at apple.com] 
> Sent: Wednesday, February 03, 2016 8:12 PM
> To: reviews+D16291+public+b25ae380e8516781 at reviews.llvm.org; Balaram Makam
> Cc: james.molloy at arm.com; t.p.northover at gmail.com; mcrosier at codeaurora.org; matze at braunis.de; junbuml at codeaurora.org; mssimpso at codeaurora.org; Bbbbb
> Subject: Re: [PATCH] D16291: AArch64: Implement missed conditional compare sequences.
> 
> Hello,
> 
> I’m experiencing problems after this commit.
> 
> The changes the check for OR formation in the DAGCombiner from:
> 
> if ((!LegalOperations || TLI.isOperationLegal(ISD::OR, VT)) &&
> 
> to
> 
> if ((!LegalOperations || TLI.isOperationLegalOrCustom(ISD::OR, VT)) &&
> 
> For us 64-bit ORs are not legal, so we have custom lowering transforming them into ADDs This combine turns them back to ORs creating a cycle in the Combiner that never terminates.
> 
> What’s the reasoning behind this change here? Making it LegalOrCustom could actually endup in a problem like this if the lowering for ORs happens to be turning into ADD and goes against the principle that usually Legal things are cheap and Custom things are not.
> 
> Marcello
> 
>> On 1 Feb 2016, at 11:17, Balaram Makam via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> 
>> This revision was automatically updated to reflect the committed changes.
>> Closed by commit rL259387: AArch64: Implement missed conditional compare sequences. (authored by bmakam).
>> 
>> Changed prior to commit:
>> http://reviews.llvm.org/D16291?vs=46546&id=46561#toc
>> 
>> Repository:
>> rL LLVM
>> 
>> http://reviews.llvm.org/D16291
>> 
>> Files:
>> llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
>> llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.h
>> llvm/trunk/test/CodeGen/AArch64/arm64-ccmp.ll
>> 
>> <D16291.46561.patch>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 
>