[llvm-commits] [PATCH] Support for non-32bit return values from CC

[Villmow, Micah]

> -----Original Message-----
> From: Rotem, Nadav [mailto:nadav.rotem at intel.com]
> Sent: Wednesday, July 25, 2012 1:26 PM
> To: Villmow, Micah; llvm-commits at cs.uiuc.edu
> Subject: RE: [PATCH] Support for non-32bit return values from CC
> 
> Generally, it looks good.  I had a few questions and comments below.
> 
> Thanks.
> 
> 
> +    SDValue SetCC1, SetCC2;
> +    if (CCCode != ISD::SETO && CCCode != ISD::SETUO) {
> +        SetCC1 = DAG.getSetCC(dl, VT, LHS, RHS, CC1);
> +        SetCC2 = DAG.getSetCC(dl, VT, LHS, RHS, CC2);
> +    } else {
> +        SetCC1 = DAG.getSetCC(dl, VT, LHS, LHS, CC1);
> +        SetCC2 = DAG.getSetCC(dl, VT, RHS, RHS, CC2);
>      }
> 
> Can you explain this part ?
[Villmow, Micah] The original code, which is the true case, handles the expansion of ordered and unordered comparisons. These comparisons are of the format (A CC1 B) Opc (A CC2 B)
The false path handles the expansion of Ordered and Unordered operations themselves which are of the format (A CC1 A) Opc (B CC2 B).
> 
> +    ISD::CondCode CCCode = cast<CondCodeSDNode>(CC)->get();
> +    EVT Tmp1VT = Tmp1.getValueType();
> 
> Maybe this will be a good time to rename Tmp1 to a meaningful name.
[Villmow, Micah] Ok
> 
> +      Tmp1 = DAG.getSetCC(dl, TLI.getSetCCResultType(Tmp1VT),
> +          Tmp1, Tmp2, CCCode);
> +      Tmp1 = DAG.getNode(Tmp1VT.isVector() ? ISD::VSELECT :
> ISD::SELECT,
> +          dl, Node->getValueType(0), Tmp1, Tmp3, Tmp4);
> 
> You can use the helper function from the other patch you sent today.
[Villmow, Micah] Yep, this is coming in a separate patch. I re-factored the code into the earlier patch after I sent this patch.
> 
> Also, how do you know that VSELECT is legal and that the condition
> operand is of the correct type ? It should be okay, since SETCC should
> be consumed by VSELECT, but I suggest that you add an assertion.
> 
> +      Tmp2 = DAG.getConstant(0, Tmp1VT);
> +      CC = DAG.getCondCode(ISD::SETNE);
> +      Tmp1 = DAG.getNode(ISD::SELECT_CC, dl, Node->getValueType(0),
> Tmp1, Tmp2,
> +          Tmp3, Tmp4, CC);
> 
> You need to check if SELECT_CC is supported.  X86 does not support it.
[Villmow, Micah] This is the code that already existed here, my backend does not support SELECT_CC either and if I did not add the true path, then the legalizer would go into a ExpandSetCC->ExpandSelectCC->ExpandSetCC loop or trigger an assert.
> 
> -    assert(Cond.getValueType().getVectorElementType() == MVT::i1 &&
> -           "Condition legalized before result?");
> 
> Why did you remove the assertion ? Can the original input mask be a non-
> i1 vector ?
> It looks like this method is used to split vector types, which means
> that we split based on the result of the node. At this point, the mask
> operand has to be vxi1.
[Villmow, Micah] getSetCCResultType() for my platform returns vx[i32|i64] not vxi1, so the assertion is not valid. Ideally my backend would be just vxi32, but I'm still working through some issues with it for 64bit types.,
The documentation for VSELECT states that it should be a vxi1, but this is only on platforms that support vxi1. There are other locations where this causes a problem that I'm still debugging. For example, when doing VSELECT on an v8i8, and a backend only supports a v4i8, the expansion of select_cc produces:
v8i1 = cmp v8i8, v8i8
v8i8 = select v8i1, v8i8, v8i8
When it attempts to split the vectors, it fails on the v8i1 since the vector is an invalid MVT type. However, if I remove the MVT::i1 restriction, I get the following:
v8i32 = cmp v8i8, v8i8
v8i8 = select v8i32, v8i8, v8i8
Which then gets split into:
v4i32 = cmp v4i8.a.lo, v4i8.b.lo
v4i8.lo = select v4i32, v4i8, v4i8
v4i32 = cmp v4i8.a.lo, v4i8.b.lo
v4i8.hi = select v4i32, v4i8, v4i8
which works fine.
Micah
> 
> 
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Villmow, Micah
> Sent: Wednesday, July 25, 2012 20:38
> To: llvm-commits at cs.uiuc.edu
> Subject: [llvm-commits] [PATCH] Support for non-32bit return values from
> CC
> 
> It looks like with LLVM 3.1, vselect was added and vsetcc and setcc were
> combined , but there are places that still assume the return value of
> the condition is i1.
> However, there is a callback getSetCCResultType, this patch adds in
> support to various locations for supporting setCC result types that are
> NOT i1.
> 
> This should apply cleanly to LLVM 3.1 but might not apply to TOT. After
> I get feedback that this approach is OK, I'll port it to TOT. This only
> shows up on backends that support vectors and don't support i1 as a
> SetCC result type.
> 
> if this approach is not ok, then I need to write lots of custom code in
> my backend to work around this issue, so early feedback is welcome!
> 
> Thanks for any feedback,
> Micah
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution by
> others is strictly prohibited. If you are not the intended recipient,
> please contact the sender and delete all copies.
>