<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta name="Generator" content="Microsoft Exchange Server">
<!-- converted from text --><style><!-- .EmailQuote { margin-left: 1pt; padding-left: 4pt; border-left: #800000 2px solid; } --></style>
</head>
<body>
<div>
<div>
<div>Yes, you can add this to 3.5.</div>
<div><br>
</div>
<div>-Hal</div>
<div><br>
</div>
<div><font style="color:#333333"><i>Sent from my Verizon Wireless 4G LTE DROID</i></font></div>
</div>
<br>
<br>
Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:<br>
<br>
</div>
<font size="2"><span style="font-size:10pt;">
<div class="PlainText">Hi,<br>
<br>
I reviewed the patch prior to submission, and it LGTM. Hal, if you<br>
agree, do you feel this is ok for 3.5 backport as well?<br>
<br>
Thanks,<br>
Bill<br>
<br>
On Fri, 2014-08-01 at 18:06 +0200, Ulrich Weigand wrote:<br>
> <br>
> Hello,<br>
> <br>
> I've run into a code-gen bug related to vector comparisons in the JIT code<br>
> generated by mesa/llvmpipe on PowerPC. The problem happens when expanding<br>
> a "fcmp uge <4 x float>" instruction (which you don't usually get from C/C+<br>
> + source code). This is currently passed to common code to expand, which<br>
> tests for unordered operands and then falls back to creating a "fcmp ge <4<br>
> x float>". This is processed by the back-end, but incorrectly.<br>
> <br>
> In getVCmpInst, we run into this case:<br>
> <br>
> case ISD::SETGE:<br>
> if (VecVT == MVT::v16i8)<br>
> return PPC::VCMPGTSB;<br>
> else if (VecVT == MVT::v8i16)<br>
> return PPC::VCMPGTSH;<br>
> else if (VecVT == MVT::v4i32)<br>
> return PPC::VCMPGTSW;<br>
> else if (VecVT == MVT::v4f32)<br>
> return HasVSX ? PPC::XVCMPGTSP : PPC::VCMPGTFP;<br>
> <br>
> so we return VCMPGTFP. But then in SelectSETCC, we have:<br>
> <br>
> case ISD::SETGE:<br>
> case ISD::SETOGE:<br>
> case ISD::SETUGE: {<br>
> // Small optimization: Altivec provides a 'Vector Compare Greater<br>
> Than<br>
> // or Equal To' instruction (vcmpgefp), so in this case there is no<br>
> // need for extra logic for the equal compare.<br>
> if (VecVT.getSimpleVT().isFloatingPoint()) {<br>
> return CurDAG->SelectNodeTo(N, VCmpInst, VecVT, LHS, RHS);<br>
> <br>
> So even though the comment says we want to emit a Compare Greater Than or<br>
> Equal To, we simply emit a Compare Greate Than.<br>
> <br>
> Note that for the SETOGE case, getVCmpInst does indeed return PPC::VCMPGTFP<br>
> -- but not for SETGE.<br>
> <br>
> Also, falling back to common code to expand SETUGE is inefficient anyway:<br>
> we could simply implement SETUGE by negating the result of a SETOLT, which<br>
> in turn is simply a SETOGT with swapped inputs. This results in just two<br>
> instructions instead of 6 or 7 the common code expander produces. (Similar<br>
> optimizations apply to SETULE, SETUGT, SETULT of course.)<br>
> <br>
> When looking at this code, I noticed a number of other cases where code<br>
> quality could be improved as well: SETOLE can be implemented as SETOGE with<br>
> swapped inputs, and for all the integer comparisons, we can do SET(U)GE by<br>
> negating the result of a SET(U)LT instead of performing two separate<br>
> comparisons for EQ and GT.<br>
> <br>
> The current structure of SelectCC / getVCmpInst makes it somewhat hard to<br>
> fix this bug and implement those enhancements, however. The two issues<br>
> that make the code more complex than it ought to be in my opinion are:<br>
> getVCmpInst tries to handle floating-point and integer comparisons using<br>
> the same logic, even though they are quite different; and, both getVCmpInst<br>
> and its caller make some decisions independently, while implicitly relying<br>
> on the other side to make the same decision.<br>
> <br>
> The attached patch fixes all this by moving all decision logic to<br>
> getVCmpInst, which gets two extra boolean outputs indicating to its caller<br>
> whether it needs to swap the input operands and/or negate the result of the<br>
> comparion. In no case do we need to create two separate comparisons any<br>
> more.<br>
> <br>
> (See attached file: diff-llvm-vcmp)<br>
> <br>
> Does this look OK?<br>
> <br>
> <br>
> Mit freundlichen Gruessen / Best Regards<br>
> <br>
> Ulrich Weigand<br>
> <br>
> --<br>
> Dr. Ulrich Weigand | Phone: +49-7031/16-3727<br>
> STSM, GNU/Linux compilers and toolchain<br>
> IBM Deutschland Research & Development GmbH<br>
> Vorsitzende des Aufsichtsrats: Martina Koederitz | Geschäftsführung: Dirk<br>
> Wittkopp<br>
> Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht<br>
> Stuttgart, HRB 243294<br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> llvm-commits@cs.uiuc.edu<br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br>
</div>
</span></font>
</body>
</html>