<div dir="ltr"><div><div>Ping ping,<br><br></div>I've explored all the options here. Can we please pick one and go with it ? The current situation is very problematic to me.<br><br></div>Thank,<br></div><div class="gmail_extra"><br><div class="gmail_quote">2014-10-02 0:31 GMT-07:00 deadal nix <span dir="ltr"><<a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>I went through all callsites of getSetCCResultType and it seems that this is the only place where it is used in that way.<br><br></div>That put into question the usefulness of the whole thing. All other getSetCCResultType are used to, in fact, get the type of setcc, and never (unless I made a mistake) for the type of predicate.<br><br></div>I'm not sure this is that useful to add a new method for simply one callsite. Matt, if your target rely on that you will have bad codegen is many scenarios. i'm not sure that is the way forward you want. We may still decide to go the new function addition road. I'm not sure what is best here. What do you all think ?<br></div><div class="gmail_extra"><br><div class="gmail_quote">2014-10-01 12:26 GMT-07:00 Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span>:<div><div class="h5"><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>----- Original Message -----<br>
> From: "deadal nix" <<a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
> Cc: "llvm-commits" <<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>>, "Nadav Rotem" <<a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a>>, "Owen Anderson" <<a href="mailto:resistor@mac.com" target="_blank">resistor@mac.com</a>>,<br>
> "Matt Arsenault" <<a href="mailto:arsenm2@gmail.com" target="_blank">arsenm2@gmail.com</a>><br>
</span><span>> Sent: Wednesday, October 1, 2014 2:13:29 PM<br>
> Subject: Re: Remove possible loop creation in DAGCombiner<br>
><br>
</span><span>> My plan was indeed to go through all the callsites and change the one<br>
> that needs to. However, this is gonna be a long and not very fun<br>
> process, so I'd like to have feedback on the approach and the<br>
> likelihood of acceptance of that change before proceeding.<br>
><br>
<br>
</span>Okay, that sounds good. The current approach confuses two separate concepts in one callback, and as you've pointed out, is buggy and insufficiently flexible. If we can refactor this in a way that is mostly-backward-compatible with the current scheme (minus the bugs), allows us additional desirable flexibility, and clearly documented, then I will certainly approve the change. If Owen objects, I'm sure he'll let us know.<br>
<br>
That having been said, I'm generally weary of half-implemented refactorings, because they often turn out to be not quite right once you attempt the rest of the implementation. So I encourage you to go through all of the callsites and make sure the proposed scheme really works; the work will not be wasted.<br>
<span><font color="#888888"><br>
 -Hal<br>
</font></span><div><div><br>
><br>
><br>
> 2014-10-01 10:18 GMT-07:00 Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> > :<br>
><br>
><br>
> ----- Original Message -----<br>
> > From: "deadal nix" < <a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a> ><br>
> > To: "Hal Finkel" < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> > Cc: "llvm-commits" < <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a> >, "Nadav Rotem" <<br>
> > <a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a> >, "Owen Anderson" < <a href="mailto:resistor@mac.com" target="_blank">resistor@mac.com</a> >,<br>
> > "Matt Arsenault" < <a href="mailto:arsenm2@gmail.com" target="_blank">arsenm2@gmail.com</a> ><br>
> > Sent: Wednesday, October 1, 2014 2:23:22 AM<br>
> > Subject: Re: Remove possible loop creation in DAGCombiner<br>
> ><br>
> ><br>
> ><br>
> > Ok I took the approach of adding a new method for the purpose at<br>
> > hand<br>
> > here.<br>
> ><br>
> > We can get that in and start spreading the use little by little.<br>
> ><br>
><br>
> I actually think that is going to be confusing. Can you please audit<br>
> the other 75 callsites of getSetCCResultType in lib/CodeGen and see<br>
> how many others should change. If we're going to do this refactoring<br>
> we should do it right and make sure it will actually work.<br>
><br>
> -Hal<br>
><br>
><br>
><br>
> ><br>
> ><br>
> > 2014-09-29 11:54 GMT-07:00 Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> > :<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > ----- Original Message -----<br>
> > > From: "deadal nix" < <a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a> ><br>
> > > To: "Hal Finkel" < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> ><br>
> ><br>
> > > Cc: "llvm-commits" < <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a> >, "Nadav Rotem" <<br>
> > > <a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a> >, "Owen Anderson" < <a href="mailto:resistor@mac.com" target="_blank">resistor@mac.com</a> >,<br>
> > > "Matt Arsenault" < <a href="mailto:arsenm2@gmail.com" target="_blank">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" target="_blank">hfinkel@anl.gov</a> > :<br>
> > ><br>
> > ><br>
> > > ----- Original Message -----<br>
> > > > From: "deadal nix" < <a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a> ><br>
> > > > To: "Hal Finkel" < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> > > > Cc: "llvm-commits" < <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a> >, "Nadav Rotem"<br>
> > > > <<br>
> > > > <a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a> >, "Owen Anderson" < <a href="mailto:resistor@mac.com" target="_blank">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<br>
> > > > 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<br>
> > > 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<br>
> > some<br>
> > additional fix somewhere.<br>
> ><br>
> > ><br>
> > ><br>
> > ><br>
> > > > - do the select with the setcc size, and then resize the<br>
> > > > 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<br>
> > behavior<br>
> > than necessary.<br>
> ><br>
> > ><br>
> > ><br>
> > > ><br>
> > > > - the proposed diff.<br>
> > > ><br>
> > > > Ultimately, the question is do the backend must handle slect<br>
> > > > 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<br>
> > > > 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<br>
> > > 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)<br>
> > > 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<br>
> > > 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>
><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div></div></div><br></div>
</blockquote></div><br></div>