
<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Oct 5, 2014, at 10:29 PM, deadal nix <<a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div>I think the best is either to leave the type intact. The new method road seemed promising at first, but the actual use case seems too limited to be worth it. I found no call site with the MVT::Other for BRCOND and no other callsite that used this for select.<br><br></div>I can prepare a patch with the documentation updated now that we know it is not used in the documented way.<br></div></blockquote><div><br></div><div>The patch as committed does not work for me and produces an invalid select with the wrong condition type. It is necessary to check the type of the select mask.</div><div><br></div><div>The select mask must be the same width as the selected operands, but the result of the setcc is the same width as the compared operands. If selecting between 32-bit values with the result of a 64-bit comparison, an invalid select with a 64-bit mask is produced.</div><div><br></div><div><br></div></div><div><blockquote type="cite"><div class="gmail_extra"><br><div class="gmail_quote">2014-10-05 22:24 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: Sunday, October 5, 2014 10:31:55 PM<br>

> Subject: Re: Remove possible loop creation in DAGCombiner<br>

><br>

</span><span class="">> Ping ping,<br>

><br>

> I've explored all the options here. Can we please pick one and go<br>

> with it ? The current situation is very problematic to me.<br>

<br>

</span>Alright, so to be clear, you think that the best solution is to leave the type of the predicate unchanged when creating the SELECT node, correct?<br>

<br>

Also, if it is just this one callsite, we should update the documentation on getSetCCResultType to make it clear it applies only to SET_CC node types. The documentation talked about using MVT::Other to get the type for BRCOND, did you not find a callsite that did that?<br>

<br>

Thanks again,<br>

Hal<br>

<div class="HOEnZb"><div class="h5"><br>

><br>

> Thank,<br>

><br>

><br>

><br>

> 2014-10-02 0:31 GMT-07:00 deadal nix < <a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a> > :<br>

><br>

><br>

><br>

><br>

><br>

> I went through all callsites of getSetCCResultType and it seems that<br>

> this is the only place where it is used in that way.<br>

><br>

> That put into question the usefulness of the whole thing. All other<br>

> getSetCCResultType are used to, in fact, get the type of setcc, and<br>

> never (unless I made a mistake) for the type of predicate.<br>

><br>

> I'm not sure this is that useful to add a new method for simply one<br>

> callsite. Matt, if your target rely on that you will have bad<br>

> codegen is many scenarios. i'm not sure that is the way forward you<br>

> want. We may still decide to go the new function addition road. I'm<br>

> not sure what is best here. What do you all think ?<br>

><br>

><br>

><br>

> 2014-10-01 12:26 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>

> > 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: Wednesday, October 1, 2014 2:13:29 PM<br>

> > Subject: Re: Remove possible loop creation in DAGCombiner<br>

> ><br>

> > My plan was indeed to go through all the callsites and change the<br>

> > 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>

> Okay, that sounds good. The current approach confuses two separate<br>

> concepts in one callback, and as you've pointed out, is buggy and<br>

> insufficiently flexible. If we can refactor this in a way that is<br>

> mostly-backward-compatible with the current scheme (minus the bugs),<br>

> allows us additional desirable flexibility, and clearly documented,<br>

> then I will certainly approve the change. If Owen objects, I'm sure<br>

> he'll let us know.<br>

><br>

> That having been said, I'm generally weary of half-implemented<br>

> refactorings, because they often turn out to be not quite right once<br>

> you attempt the rest of the implementation. So I encourage you to go<br>

> through all of the callsites and make sure the proposed scheme<br>

> really works; the work will not be wasted.<br>

><br>

> -Hal<br>

><br>

><br>

><br>

> ><br>

> ><br>

> > 2014-10-01 10:18 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>

> > > "Matt Arsenault" < <a href="mailto:arsenm2@gmail.com">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<br>

> > 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<br>

> > 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">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>

> > > > <<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<br>

> > > > > Rotem"<br>

> > > > > <<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<br>

> > > > > 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<br>

> > > > > 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<br>

> > > > 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,<br>

> > > > 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<br>

> > > > > 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<br>

> > > > 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<br>

> > > > > result<br>

> > > > > of<br>

> > > > > an incorrect use of that function will be a source of<br>

> > > > > trouble.<br>

> > > ><br>

> > > > This is unfortunately somewhat confusing. getSetCCResultType is<br>

> > > > documented as:<br>

> > > ><br>

> > > > /// Return the ValueType of the result of SETCC operations.<br>

> > > > Also<br>

> > > > used<br>

> > > > to<br>

> > > > /// obtain the target's preferred type for the condition<br>

> > > > 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"<br>

> > > > 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<br>

> > > > 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,<br>

> > > > 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<br>

> > > > backend<br>

> > > > need to have information about the type of the predicate and<br>

> > > > 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>

><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>

</blockquote></div><br></body></html>