[llvm] r259387 - AArch64: Implement missed conditional compare sequences.

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 6 16:33:49 PDT 2016


Hi Balaram,

> On Jun 6, 2016, at 2:43 PM, Balaram Makam <bmakam at codeaurora.org> wrote:
> 
> Hi Quentin,
>  
> I will take another look at this. I believe the problem is seen only on the internal branch. Do you have a test case to reproduce the issue?

Unfortunately, no, I do not have a test case for you.
Basically, this patch generates illegal code after legalization, and the problem is only the fact that we use Custom here to check legality.

You should be able to achieve the same combine in the AArch64 backend with just the isLegal part of the check.
There are a bunch of hooks during the lowering of SDAG and I am pretty sure you could find another way to do your custom lowering (I know I’ve already been in that situation :)). The "perform dag combine” that I mentioned should be one of them, but you have others.

Cheers,
-Quentin

>  
> Thanks,
> Balaram
>  
> From: Quentin Colombet [mailto:qcolombet at apple.com] 
> Sent: Monday, June 06, 2016 5:30 PM
> To: Balaram Makam <bmakam at codeaurora.org>
> Cc: llvm-commits at lists.llvm.org
> Subject: Re: [llvm] r259387 - AArch64: Implement missed conditional compare sequences.
>  
> Hi Balaram,
>  
> Sorry for the late reply, but I am seeing this commit is causing problem internally.
>  
>> On Feb 1, 2016, at 11:13 AM, Balaram Makam via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>  
>> Author: bmakam
>> Date: Mon Feb  1 13:13:07 2016
>> New Revision: 259387
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=259387&view=rev <http://llvm.org/viewvc/llvm-project?rev=259387&view=rev>
>> Log:
>> AArch64: Implement missed conditional compare sequences.
>> 
>> Summary:
>> This is an extension to the existing implementation of r242436 which
>> restricts to only select inputs. This version fixes missed opportunities
>> in pr26084 by attempting to lower conditional compare sequences of
>> and/or trees with setcc leafs. This will additionaly handle the case
>> when a tree with select input is not a conjunction-disjunction tree
>> but some of the sub trees are conjunction-disjunction trees.
>> 
>> Reviewers: jmolloy, t.p.northover, mcrosier, MatzeB
>> 
>> Subscribers: mcrosier, llvm-commits, junbuml, haicheng, mssimpso, gberry
>> 
>> Differential Revision: http://reviews.llvm.org/D16291 <http://reviews.llvm.org/D16291>
>> 
>> Modified:
>>    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
>> 
>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=259387&r1=259386&r2=259387&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=259387&r1=259386&r2=259387&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Mon Feb  1 13:13:07 2016
>> @@ -1721,7 +1721,7 @@ SDValue DAGCombiner::visitADD(SDNode *N)
>>     return SDValue(N, 0);
>> 
>>   // fold (a+b) -> (a|b) iff a and b share no bits.
>> -  if ((!LegalOperations || TLI.isOperationLegal(ISD::OR, VT)) &&
>> +  if ((!LegalOperations || TLI.isOperationLegalOrCustom(ISD::OR, VT)) &&
>  
> This is wrong. Custom does not imply that the operation is Legal!
>  
> You said in the review that without custom, one of the lit test case was failing. I believe you can fix that by making the Custom lowering as part of the combine process (PerformDAGCombine.)
>  
> Anyhow, this line is generally not correct.
>  
> Cheers,
> -Quentin
>  
> 
> 
>>       VT.isInteger() && !VT.isVector() && DAG.haveNoCommonBitsSet(N0, N1))
>>     return DAG.getNode(ISD::OR, SDLoc(N), VT, N0, N1);
>> 
>> @@ -6363,7 +6363,7 @@ SDValue DAGCombiner::visitZERO_EXTEND(SD
>>       isa<LoadSDNode>(N0.getOperand(0)) &&
>>       N0.getOperand(1).getOpcode() == ISD::Constant &&
>>       TLI.isLoadExtLegal(ISD::ZEXTLOAD, VT, N0.getValueType()) &&
>> -      (!LegalOperations && TLI.isOperationLegal(N0.getOpcode(), VT))) {
>> +      (!LegalOperations && TLI.isOperationLegalOrCustom(N0.getOpcode(), VT))) {
>>     LoadSDNode *LN0 = cast<LoadSDNode>(N0.getOperand(0));
>>     if (LN0->getExtensionType() != ISD::SEXTLOAD && LN0->isUnindexed()) {
>>       bool DoXform = true;
>> 
>> Modified: llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?rev=259387&r1=259386&r2=259387&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?rev=259387&r1=259386&r2=259387&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp (original)
>> +++ llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp Mon Feb  1 13:13:07 2016
>> @@ -144,6 +144,16 @@ AArch64TargetLowering::AArch64TargetLowe
>>   setOperationAction(ISD::XOR, MVT::i32, Custom);
>>   setOperationAction(ISD::XOR, MVT::i64, Custom);
>> 
>> +  // Custom lowering hooks are needed for OR
>> +  // to fold it into CCMP.
>> +  setOperationAction(ISD::OR, MVT::i32, Custom);
>> +  setOperationAction(ISD::OR, MVT::i64, Custom);
>> +
>> +  // Custom lowering hooks are needed for AND
>> +  // to fold it into CCMP.
>> +  setOperationAction(ISD::AND, MVT::i32, Custom);
>> +  setOperationAction(ISD::AND, MVT::i64, Custom);
>> +
>>   // Virtually no operation on f128 is legal, but LLVM can't expand them when
>>   // there's a valid register class, so we need custom operations in most cases.
>>   setOperationAction(ISD::FABS, MVT::f128, Expand);
>> @@ -1597,6 +1607,27 @@ static SDValue getAArch64Cmp(SDValue LHS
>>   return Cmp;
>> }
>> 
>> +// Attempt to form conditional compare sequences for and/or trees
>> +// with setcc leafs.
>> +static SDValue tryLowerToAArch64Cmp(SDValue Op, SelectionDAG &DAG) {
>> +  SDValue LHS = Op.getOperand(0);
>> +  SDValue RHS = Op.getOperand(1);
>> +  if ((LHS.getOpcode() != ISD::SETCC) || (RHS.getOpcode() != ISD::SETCC))
>> +    return Op;
>> +
>> +  bool CanNegate;
>> +  if (!isConjunctionDisjunctionTree(Op, CanNegate))
>> +    return SDValue();
>> +
>> +  EVT VT = Op.getValueType();
>> +  SDLoc DL(Op);
>> +  SDValue TVal = DAG.getConstant(1, DL, VT);
>> +  SDValue FVal = DAG.getConstant(0, DL, VT);
>> +  SDValue CCVal;
>> +  SDValue Cmp = getAArch64Cmp(Op, FVal, ISD::SETEQ, CCVal, DAG, DL);
>> +  return DAG.getNode(AArch64ISD::CSEL, DL, VT, FVal, TVal, CCVal, Cmp);
>> +}
>> +
>> static std::pair<SDValue, SDValue>
>> getAArch64XALUOOp(AArch64CC::CondCode &CC, SDValue Op, SelectionDAG &DAG) {
>>   assert((Op.getValueType() == MVT::i32 || Op.getValueType() == MVT::i64) &&
>> @@ -1718,6 +1749,18 @@ SDValue AArch64TargetLowering::LowerF128
>>   return makeLibCall(DAG, Call, MVT::f128, Ops, false, SDLoc(Op)).first;
>> }
>> 
>> +SDValue AArch64TargetLowering::LowerAND(SDValue Op, SelectionDAG &DAG) const {
>> +  if (Op.getValueType().isVector())
>> +    return LowerVectorAND(Op, DAG);
>> +  return tryLowerToAArch64Cmp(Op, DAG);
>> +}
>> +
>> +SDValue AArch64TargetLowering::LowerOR(SDValue Op, SelectionDAG &DAG) const {
>> +  if (Op.getValueType().isVector())
>> +    return LowerVectorOR(Op, DAG);
>> +  return tryLowerToAArch64Cmp(Op, DAG);
>> +}
>> +
>> static SDValue LowerXOR(SDValue Op, SelectionDAG &DAG) {
>>   SDValue Sel = Op.getOperand(0);
>>   SDValue Other = Op.getOperand(1);
>> @@ -2372,9 +2415,9 @@ SDValue AArch64TargetLowering::LowerOper
>>   case ISD::FCOPYSIGN:
>>     return LowerFCOPYSIGN(Op, DAG);
>>   case ISD::AND:
>> -    return LowerVectorAND(Op, DAG);
>> +    return LowerAND(Op, DAG);
>>   case ISD::OR:
>> -    return LowerVectorOR(Op, DAG);
>> +    return LowerOR(Op, DAG);
>>   case ISD::XOR:
>>     return LowerXOR(Op, DAG);
>>   case ISD::PREFETCH:
>> 
>> Modified: llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.h?rev=259387&r1=259386&r2=259387&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.h?rev=259387&r1=259386&r2=259387&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.h (original)
>> +++ llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.h Mon Feb  1 13:13:07 2016
>> @@ -488,6 +488,8 @@ private:
>>   SDValue LowerCTPOP(SDValue Op, SelectionDAG &DAG) const;
>>   SDValue LowerF128Call(SDValue Op, SelectionDAG &DAG,
>>                         RTLIB::Libcall Call) const;
>> +  SDValue LowerAND(SDValue Op, SelectionDAG &DAG) const;
>> +  SDValue LowerOR(SDValue Op, SelectionDAG &DAG) const;
>>   SDValue LowerFCOPYSIGN(SDValue Op, SelectionDAG &DAG) const;
>>   SDValue LowerFP_EXTEND(SDValue Op, SelectionDAG &DAG) const;
>>   SDValue LowerFP_ROUND(SDValue Op, SelectionDAG &DAG) const;
>> 
>> Modified: llvm/trunk/test/CodeGen/AArch64/arm64-ccmp.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/arm64-ccmp.ll?rev=259387&r1=259386&r2=259387&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/arm64-ccmp.ll?rev=259387&r1=259386&r2=259387&view=diff>
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/AArch64/arm64-ccmp.ll (original)
>> +++ llvm/trunk/test/CodeGen/AArch64/arm64-ccmp.ll Mon Feb  1 13:13:07 2016
>> @@ -371,21 +371,76 @@ define i32 @select_andor(i32 %v1, i32 %v
>>   ret i32 %sel
>> }
>> 
>> -; CHECK-LABEL: select_noccmp1
>> -define i64 @select_noccmp1(i64 %v1, i64 %v2, i64 %v3, i64 %r) {
>> -; CHECK: cmp x0, #0
>> -; CHECK-NEXT: cset [[REG0:w[0-9]+]], lt
>> -; CHECK-NEXT: cmp x0, #13
>> -; CHECK-NOT: ccmp
>> +; CHECK-LABEL: single_noselect
>> +define i32 @single_noselect(i32 %A, i32 %B) #0 {
>> +; CHECK: cmp w1, #1
>> +; CHECK-NEXT: ccmp w0, #1, #8, ge
>> +; CHECK-NEXT: cset w0, lt
>> +; CHECK-NEXT: ret
>> +  %notlhs = icmp slt i32 %A, 1
>> +  %notrhs = icmp slt i32 %B, 1
>> +  %lnot = or i1 %notlhs, %notrhs
>> +  %conv = zext i1 %lnot to i32
>> +  ret i32 %conv
>> +}
>> +
>> +; CHECK-LABEL: single_and_ext
>> +define i32 @single_and_ext(i32 %A, i32 %B, i32 %C) #0 {
>> +; CHECK: cmp w1, #2
>> +; CHECK-NEXT: ccmp w0, #4, #0, lt
>> +; CHECK-NEXT: cinc w0, w2, lt
>> +; CHECK-NEXT: ret
>> +  %cmp = icmp slt i32 %A, 4
>> +  %cmp1 = icmp slt i32 %B, 2
>> +  %and1 = and i1 %cmp, %cmp1
>> +  %conv = zext i1 %and1 to i32
>> +  %add = add nsw i32 %conv, %C
>> +  ret i32 %add
>> +}
>> +
>> +; CHECK-LABEL: single_noselect_phi
>> +define i32 @single_noselect_phi(i32 %A, i32 %B, i32 %C) #0 {
>> +; CHECK: cmp w1, #0
>> +; CHECK-NEXT: ccmp w0, #0, #4, gt
>> ; CHECK-NEXT: cset [[REG1:w[0-9]+]], gt
>> -; CHECK-NEXT: cmp x2, #2
>> +; CHECK-NEXT: cmp w1, #2
>> +; CHECK-NEXT: ccmp w0, #4, #8, ge
>> ; CHECK-NEXT: cset [[REG2:w[0-9]+]], lt
>> +; CHECK-NEXT: cmp w2, #0
>> +; CHECK-NEXT: csel w0, [[REG1]], [[REG2]], eq
>> +; CHECK-NEXT: ret
>> +entry:
>> +  %tobool = icmp eq i32 %C, 0
>> +  br i1 %tobool, label %if.else, label %if.then
>> +
>> +if.then:                                          ; preds = %entry
>> +  %cmp = icmp slt i32 %A, 4
>> +  %cmp1 = icmp slt i32 %B, 2
>> +  %0 = or i1 %cmp, %cmp1
>> +  br label %if.end
>> +
>> +if.else:                                          ; preds = %entry
>> +  %cmp2 = icmp sgt i32 %A, 0
>> +  %cmp3 = icmp sgt i32 %B, 0
>> +  %1 = and i1 %cmp2, %cmp3
>> +  br label %if.end
>> +
>> +if.end:                                           ; preds = %if.else, %if.then
>> +  %b.0.in = phi i1 [ %0, %if.then ], [ %1, %if.else ]
>> +  %conv = zext i1 %b.0.in to i32
>> +  ret i32 %conv
>> +}
>> +
>> +; CHECK-LABEL: select_noccmp1
>> +define i64 @select_noccmp1(i64 %v1, i64 %v2, i64 %v3, i64 %r) {
>> +; CHECK: cmp x0, #13
>> +; CHECK-NEXT: ccmp x0, #0, #0, gt
>> +; CHECK-NEXT: cset [[REG1:w[0-9]+]], lt
>> ; CHECK-NEXT: cmp x2, #4
>> -; CHECK-NEXT: cset [[REG3:w[0-9]+]], gt
>> -; CHECK-NEXT: and [[REG4:w[0-9]+]], [[REG0]], [[REG1]]
>> -; CHECK-NEXT: and [[REG5:w[0-9]+]], [[REG2]], [[REG3]]
>> -; CHECK-NEXT: orr [[REG6:w[0-9]+]], [[REG4]], [[REG5]]
>> -; CHECK-NEXT: cmp [[REG6]], #0
>> +; CHECK-NEXT: ccmp x2, #2, #0, gt
>> +; CHECK-NEXT: cset [[REG2:w[0-9]+]], lt
>> +; CHECK-NEXT: orr [[REG3:w[0-9]+]], [[REG1]], [[REG2]]
>> +; CHECK-NEXT: cmp [[REG3]], #0
>> ; CHECK-NEXT: csel x0, xzr, x3, ne
>> ; CHECK-NEXT: ret
>>   %c0 = icmp slt i64 %v1, 0
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160606/3d22733e/attachment.html>


More information about the llvm-commits mailing list