<div dir="ltr">My plan was indeed to go through all the callsites and change the one that needs to. However, this is gonna be a long and not very fun process, so I'd like to have feedback on the approach and the likelihood of acceptance of that change before proceeding.<br></div><div class="gmail_extra"><br><div class="gmail_quote">2014-10-01 10:18 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"><span class="">----- Original Message -----<br>
> From: "deadal nix" <<a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> Cc: "llvm-commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>>, "Nadav Rotem" <<a href="mailto:nrotem@apple.com">nrotem@apple.com</a>>, "Owen Anderson" <<a href="mailto:resistor@mac.com">resistor@mac.com</a>>,<br>
> "Matt Arsenault" <<a href="mailto:arsenm2@gmail.com">arsenm2@gmail.com</a>><br>
</span><span class="">> Sent: Wednesday, October 1, 2014 2:23:22 AM<br>
> Subject: Re: Remove possible loop creation in DAGCombiner<br>
><br>
><br>
><br>
</span><span class="">> Ok I took the approach of adding a new method for the purpose at hand<br>
> here.<br>
><br>
> We can get that in and start spreading the use little by little.<br>
><br>
<br>
</span>I actually think that is going to be confusing. Can you please audit the other 75 callsites of getSetCCResultType in lib/CodeGen and see how many others should change. If we're going to do this refactoring we should do it right and make sure it will actually work.<br>
<span class="HOEnZb"><font color="#888888"><br>
 -Hal<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> 2014-09-29 11:54 GMT-07:00 Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> > :<br>
><br>
><br>
><br>
><br>
> ----- Original Message -----<br>
> > From: "deadal nix" < <a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a> ><br>
> > To: "Hal Finkel" < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
><br>
><br>
> > Cc: "llvm-commits" < <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> >, "Nadav Rotem" <<br>
> > <a href="mailto:nrotem@apple.com">nrotem@apple.com</a> >, "Owen Anderson" < <a href="mailto:resistor@mac.com">resistor@mac.com</a> >,<br>
> > "Matt Arsenault" < <a href="mailto:arsenm2@gmail.com">arsenm2@gmail.com</a> ><br>
> > Sent: Sunday, September 28, 2014 2:42:10 AM<br>
> > Subject: Re: Remove possible loop creation in DAGCombiner<br>
> ><br>
> > 2014-09-28 0:12 GMT-07:00 Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> > :<br>
> ><br>
> ><br>
> > ----- Original Message -----<br>
> > > From: "deadal nix" < <a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a> ><br>
> > > To: "Hal Finkel" < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> > > Cc: "llvm-commits" < <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> >, "Nadav Rotem" <<br>
> > > <a href="mailto:nrotem@apple.com">nrotem@apple.com</a> >, "Owen Anderson" < <a href="mailto:resistor@mac.com">resistor@mac.com</a> ><br>
> > > Sent: Friday, September 26, 2014 3:46:32 AM<br>
> > > Subject: Re: Remove possible loop creation in DAGCombiner<br>
> > ><br>
> > > I tried various options:<br>
> > ><br>
> > > - Not do the transformation when size do not match. This break<br>
> > > several backends as the sext is done from a 1bit value which most<br>
> > > backend do not expect or support. This needs to be transformed<br>
> > > into<br>
> > > a select one way or another.<br>
> ><br>
> > This seems odd, type legalization should take care of that problem<br>
> > (and DAGCombine should not introduce illegal types after type<br>
> > legalization is complete). What transformation in producing these<br>
> > illegal nodes?<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > The problem is that the sext from a setcc node is unexpected by<br>
> > several in tree backends. I didn't invetigated in details, simply<br>
> > noticed that there was a lot of broken ciodegen tests .<br>
><br>
> Can you please revisit this approach? This really seems like the<br>
> right solution (perhaps unfortunately), even if it does require some<br>
> additional fix somewhere.<br>
><br>
> ><br>
> ><br>
> ><br>
> > > - do the select with the setcc size, and then resize the select.<br>
> > > It<br>
> > > does work but produce inefficient code in various situations<br>
> > > (select<br>
> > > a 64 bit and resize the result to 32bits).<br>
> > ><br>
> > > - trunc but never extend. This yield the same results as the<br>
> > > proposed<br>
> > > diff.<br>
> ><br>
> > What does "same results" mean?<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > That the codegen were the same for all tests I looked at.<br>
> ><br>
><br>
> Fair enough, but I have no idea how good the coverage is, so the<br>
> conservative impulse in me says to not change more existing behavior<br>
> than necessary.<br>
><br>
> ><br>
> ><br>
> > ><br>
> > > - the proposed diff.<br>
> > ><br>
> > > Ultimately, the question is do the backend must handle slect with<br>
> > > different size for selected values and predicate. It look like<br>
> > > they<br>
> > > do (and other part of the toolchain generate such code).<br>
> ><br>
> > As documented, the sizes of the selected values and the predicate<br>
> > may<br>
> > not match. include/llvm/CodeGen/ISDOpcodes.h says:<br>
> ><br>
> > /// Select(COND, TRUEVAL, FALSEVAL). If the type of the boolean<br>
> > COND<br>
> > is not<br>
> > /// i1 then the high bits must conform to getBooleanContents.<br>
> > SELECT,<br>
> ><br>
> > and so the SIGN_EXTEND there is generically wrong anyway (it is<br>
> > correct only for ZeroOrNegativeOneBooleanContent).<br>
> ><br>
> > Then I guess we should simply not do it.<br>
><br>
> Maybe ;) -- or do it correctly: sign extend for<br>
> ZeroOrNegativeOneBooleanContent, zero extend for the others.<br>
><br>
><br>
><br>
> ><br>
> ><br>
> ><br>
> > ><br>
> > > Another thing worth noticing is the use of getSetCCResultType<br>
> > > with<br>
> > > the selected value's type as parameter. This is a different<br>
> > > semantic<br>
> > > than other uses of the method (predicate's argument type is<br>
> > > passed).<br>
> > > It seems that this is an incorrect use of that function to begin<br>
> > > with, as there is no reason that the 2 respond to the same<br>
> > > constraint, and, in the case of the select, this is not given<br>
> > > that<br>
> > > there is a constraint at all. Basing any decision on the result<br>
> > > of<br>
> > > an incorrect use of that function will be a source of trouble.<br>
> ><br>
> > This is unfortunately somewhat confusing. getSetCCResultType is<br>
> > documented as:<br>
> ><br>
> > /// Return the ValueType of the result of SETCC operations. Also<br>
> > used<br>
> > to<br>
> > /// obtain the target's preferred type for the condition operand of<br>
> > SELECT and<br>
> > /// BRCOND nodes. In the case of BRCOND the argument passed is<br>
> > MVT::Other<br>
> > /// since there are no other operands to get a type hint from.<br>
> > virtual EVT getSetCCResultType(LLVMContext &Context, EVT VT) const;<br>
> ><br>
> > so it seems to be a matter of determining what "preference" means<br>
> > here, and whether backends deal reasonably with non-preferred<br>
> > choices.<br>
> ><br>
> > Thanks again,<br>
> > Hal<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > That do not make a lot of sense to me. There is no reason for a<br>
> > target to have the same preference in both scenarios.<br>
><br>
> I agree with this. Please feel free to submit patches to improve<br>
> this.<br>
><br>
> Thanks again,<br>
> Hal<br>
><br>
> > In fact the<br>
> > one I'm working on will generate a result that has the same size as<br>
> > argument for the setcc, and do not care about the size of the<br>
> > predicate. If I select a 64bits values from a 32bits predicate, I<br>
> > get a loop in the dag:<br>
> ><br>
> ><br>
> > (sext i64 (setcc i32:X i32:Y)) =>(select (sext i64 (setcc i32:X<br>
> > i32:Y)) -1 0)<br>
> ><br>
> ><br>
> > When you proceed to the substitution, you end up with a loop:<br>
> ><br>
> > N = (select N -1 0)<br>
> ><br>
> ><br>
> > To make the right decision in the case of the select, the backend<br>
> > need to have information about the type of the predicate and the<br>
> > type of the selected values. One of them is insufficient.<br>
> ><br>
><br>
><br>
><br>
> --<br>
> Hal Finkel<br>
> Assistant Computational Scientist<br>
> Leadership Computing Facility<br>
> Argonne National Laboratory<br>
><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div>