<div dir="ltr"><div><div>I tried various options:<br></div><div> - Not do the transformation when size do not match. This break several backends as the sext is done from a 1bit value which most backend do not expect or support. This needs to be transformed into a select one way or another.<br></div> - do the select with the setcc size, and then resize the select. It does work but produce inefficient code in various situations (select a 64 bit and resize the result to 32bits).<br></div><div><div> - trunc but never extend. This yield the same results as the proposed diff.<br></div><div> - the proposed diff.<br><br></div><div>Ultimately, the question is do the backend must handle slect with different size for selected values and predicate. It look like they do (and other part of the toolchain generate such code).<br><br></div><div>Another thing worth noticing is the use of getSetCCResultType with the selected value's type as parameter. This is a different semantic than other uses of the method (predicate's argument type is passed). It seems that this is an incorrect use of that function to begin with, as there is no reason that the 2 respond to the same constraint, and, in the case of the select, this is not given that there is a constraint at all. Basing any decision on the result of an incorrect use of that function will be a source of trouble.<br><br></div><div>If such information is really required, another method should be used, but as this is the only use case I've found in the codebase, and that other part of it generate select without this, I'm inclined to assume that this is not something commonly used. It is not used line 11422 in the same file for instance.<br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">2014-09-25 13:42 GMT-07:00 Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">----- Original Message -----<br>
> From: "deadal nix" <<a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a>><br>
> To: "llvm-commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
> Sent: Thursday, September 25, 2014 12:09:07 AM<br>
> Subject: Remove possible loop creation in DAGCombiner<br>
><br>
><br>
><br>
><br>
> DAGCombiner can create loop in the DAG when you sext the result of a<br>
> setcc, depending on the target.<br>
><br>
> This diff remove the call to DAG.getSExtOrTrunc that create the loop.<br>
> This call used to be necessary for x86 backend, but it doesn't look<br>
> necessary anymore. I've added tests to ensure it doesn't.<br>
<br>
</div></div>- EVT SelectVT = getSetCCResultType(VT);<br>
- return DAG.getSelect(DL, VT,<br>
- DAG.getSExtOrTrunc(SetCC, DL, SelectVT),<br>
- NegOne, DAG.getConstant(0, VT));<br>
-<br>
+ return DAG.getSelect(DL, VT, SetCC,<br>
+ NegOne, DAG.getConstant(0, VT));<br>
}<br>
<br>
It seems clear to me that creating an SIGN_EXTEND inside of visitSIGN_EXTEND could easily lead to a loop, and we should avoid that. However, getSExtOrTrunc might also have been providing a TRUNC, and if the SIGN_EXTEND really would be needed, we may just want to abort the transformation all together. For one thing, I don't think that it can be different unless VT != N0.getOperand(0).getValueType().<br>
<br>
In short, you're proposing to change target-independent code, and I'd be more conservative. Why don't you just disable the transformation when SelectVT.bitsGT(SetCC.getValueType()) is true (which I believe is the condition under which we'd get the SIGN_EXTEND).<br>
<br>
-Hal<br>
<br>
><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div>