Remove possible loop creation in DAGCombiner

Mon Oct 6 16:05:26 PDT 2014

Matt, it is solvable in two ways, depending how common the problem is:
 - If it is a very common problem, we can resurrect the method I proposed
in previous patch to select predicate size (and the target will have enough
infos to avoid creating a loop). But this will require important
refactoring as right now, literally no select condition is resized. That is
an important refactoring, but definitely doable.
 - If it is limited to a limited amount of targets, then Create specific
matching rule for the target so the right code is generated.

Considering #1 is an important change, I'll let core devs to decide if it
is worth it and a direction LLVM want to go in.

2014-10-06 15:26 GMT-07:00 Matt Arsenault <arsenm2 at gmail.com>:

>
> On Oct 5, 2014, at 10:29 PM, deadal nix <deadalnix at gmail.com> wrote:
>
> 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.
>
> I can prepare a patch with the documentation updated now that we know it
> is not used in the documented way.
>
>
> 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.
>
> 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.
>
>
>
> 2014-10-05 22:24 GMT-07:00 Hal Finkel <hfinkel at anl.gov>:
>
>> ----- Original Message -----
>> > From: "deadal nix" <deadalnix at gmail.com>
>> > To: "Hal Finkel" <hfinkel at anl.gov>
>> > Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>, "Nadav Rotem" <
>> nrotem at apple.com>, "Owen Anderson" <resistor at mac.com>,
>> > "Matt Arsenault" <arsenm2 at gmail.com>
>> > Sent: Sunday, October 5, 2014 10:31:55 PM
>> > Subject: Re: Remove possible loop creation in DAGCombiner
>> >
>> > Ping ping,
>> >
>> > I've explored all the options here. Can we please pick one and go
>> > with it ? The current situation is very problematic to me.
>>
>> 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?
>>
>> 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?
>>
>> Thanks again,
>> Hal
>>
>> >
>> > Thank,
>> >
>> >
>> >
>> > 2014-10-02 0:31 GMT-07:00 deadal nix < deadalnix at gmail.com > :
>> >
>> >
>> >
>> >
>> >
>> > I went through all callsites of getSetCCResultType and it seems that
>> > this is the only place where it is used in that way.
>> >
>> > 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.
>> >
>> > 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 ?
>> >
>> >
>> >
>> > 2014-10-01 12:26 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
>> >
>> >
>> >
>> >
>> > ----- Original Message -----
>> > > From: "deadal nix" < deadalnix at gmail.com >
>> > > To: "Hal Finkel" < hfinkel at anl.gov >
>> > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu >, "Nadav Rotem" <
>> > > nrotem at apple.com >, "Owen Anderson" < resistor at mac.com >,
>> > > "Matt Arsenault" < arsenm2 at gmail.com >
>> > > Sent: Wednesday, October 1, 2014 2:13:29 PM
>> > > Subject: Re: Remove possible loop creation in DAGCombiner
>> > >
>> > > 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.
>> > >
>> >
>> > 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.
>> >
>> > 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.
>> >
>> > -Hal
>> >
>> >
>> >
>> > >
>> > >
>> > > 2014-10-01 10:18 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
>> > >
>> > >
>> > > ----- Original Message -----
>> > > > From: "deadal nix" < deadalnix at gmail.com >
>> > > > To: "Hal Finkel" < hfinkel at anl.gov >
>> > > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu >, "Nadav Rotem" <
>> > > > nrotem at apple.com >, "Owen Anderson" < resistor at mac.com >,
>> > > > "Matt Arsenault" < arsenm2 at gmail.com >
>> > > > Sent: Wednesday, October 1, 2014 2:23:22 AM
>> > > > Subject: Re: Remove possible loop creation in DAGCombiner
>> > > >
>> > > >
>> > > >
>> > > > Ok I took the approach of adding a new method for the purpose at
>> > > > hand
>> > > > here.
>> > > >
>> > > > We can get that in and start spreading the use little by little.
>> > > >
>> > >
>> > > 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.
>> > >
>> > > -Hal
>> > >
>> > >
>> > >
>> > > >
>> > > >
>> > > > 2014-09-29 11:54 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
>> > > >
>> > > >
>> > > >
>> > > >
>> > > > ----- Original Message -----
>> > > > > From: "deadal nix" < deadalnix at gmail.com >
>> > > > > To: "Hal Finkel" < hfinkel at anl.gov >
>> > > >
>> > > >
>> > > > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu >, "Nadav Rotem"
>> > > > > <
>> > > > > nrotem at apple.com >, "Owen Anderson" < resistor at mac.com >,
>> > > > > "Matt Arsenault" < arsenm2 at gmail.com >
>> > > > > Sent: Sunday, September 28, 2014 2:42:10 AM
>> > > > > Subject: Re: Remove possible loop creation in DAGCombiner
>> > > > >
>> > > > > 2014-09-28 0:12 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
>> > > > >
>> > > > >
>> > > > > ----- Original Message -----
>> > > > > > From: "deadal nix" < deadalnix at gmail.com >
>> > > > > > To: "Hal Finkel" < hfinkel at anl.gov >
>> > > > > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu >, "Nadav
>> > > > > > Rotem"
>> > > > > > <
>> > > > > > nrotem at apple.com >, "Owen Anderson" < resistor at mac.com >
>> > > > > > Sent: Friday, September 26, 2014 3:46:32 AM
>> > > > > > Subject: Re: Remove possible loop creation in DAGCombiner
>> > > > > >
>> > > > > > I tried various options:
>> > > > > >
>> > > > > > - 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.
>> > > > >
>> > > > > This seems odd, type legalization should take care of that
>> > > > > problem
>> > > > > (and DAGCombine should not introduce illegal types after type
>> > > > > legalization is complete). What transformation in producing
>> > > > > these
>> > > > > illegal nodes?
>> > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > The problem is that the sext from a setcc node is unexpected by
>> > > > > several in tree backends. I didn't invetigated in details,
>> > > > > simply
>> > > > > noticed that there was a lot of broken ciodegen tests .
>> > > >
>> > > > Can you please revisit this approach? This really seems like the
>> > > > right solution (perhaps unfortunately), even if it does require
>> > > > some
>> > > > additional fix somewhere.
>> > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > > - 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).
>> > > > > >
>> > > > > > - trunc but never extend. This yield the same results as the
>> > > > > > proposed
>> > > > > > diff.
>> > > > >
>> > > > > What does "same results" mean?
>> > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > That the codegen were the same for all tests I looked at.
>> > > > >
>> > > >
>> > > > Fair enough, but I have no idea how good the coverage is, so the
>> > > > conservative impulse in me says to not change more existing
>> > > > behavior
>> > > > than necessary.
>> > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > > - the proposed diff.
>> > > > > >
>> > > > > > 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).
>> > > > >
>> > > > > As documented, the sizes of the selected values and the
>> > > > > predicate
>> > > > > may
>> > > > > not match. include/llvm/CodeGen/ISDOpcodes.h says:
>> > > > >
>> > > > > /// Select(COND, TRUEVAL, FALSEVAL). If the type of the boolean
>> > > > > COND
>> > > > > is not
>> > > > > /// i1 then the high bits must conform to getBooleanContents.
>> > > > > SELECT,
>> > > > >
>> > > > > and so the SIGN_EXTEND there is generically wrong anyway (it is
>> > > > > correct only for ZeroOrNegativeOneBooleanContent).
>> > > > >
>> > > > > Then I guess we should simply not do it.
>> > > >
>> > > > Maybe ;) -- or do it correctly: sign extend for
>> > > > ZeroOrNegativeOneBooleanContent, zero extend for the others.
>> > > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > > 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.
>> > > > >
>> > > > > This is unfortunately somewhat confusing. getSetCCResultType is
>> > > > > documented as:
>> > > > >
>> > > > > /// Return the ValueType of the result of SETCC operations.
>> > > > > Also
>> > > > > used
>> > > > > to
>> > > > > /// obtain the target's preferred type for the condition
>> > > > > operand
>> > > > > of
>> > > > > SELECT and
>> > > > > /// BRCOND nodes. In the case of BRCOND the argument passed is
>> > > > > MVT::Other
>> > > > > /// since there are no other operands to get a type hint from.
>> > > > > virtual EVT getSetCCResultType(LLVMContext &Context, EVT VT)
>> > > > > const;
>> > > > >
>> > > > > so it seems to be a matter of determining what "preference"
>> > > > > means
>> > > > > here, and whether backends deal reasonably with non-preferred
>> > > > > choices.
>> > > > >
>> > > > > Thanks again,
>> > > > > Hal
>> > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > That do not make a lot of sense to me. There is no reason for a
>> > > > > target to have the same preference in both scenarios.
>> > > >
>> > > > I agree with this. Please feel free to submit patches to improve
>> > > > this.
>> > > >
>> > > > Thanks again,
>> > > > Hal
>> > > >
>> > > > > In fact the
>> > > > > one I'm working on will generate a result that has the same
>> > > > > size
>> > > > > as
>> > > > > argument for the setcc, and do not care about the size of the
>> > > > > predicate. If I select a 64bits values from a 32bits predicate,
>> > > > > I
>> > > > > get a loop in the dag:
>> > > > >
>> > > > >
>> > > > > (sext i64 (setcc i32:X i32:Y)) =>(select (sext i64 (setcc i32:X
>> > > > > i32:Y)) -1 0)
>> > > > >
>> > > > >
>> > > > > When you proceed to the substitution, you end up with a loop:
>> > > > >
>> > > > > N = (select N -1 0)
>> > > > >
>> > > > >
>> > > > > To make the right decision in the case of the select, the
>> > > > > backend
>> > > > > need to have information about the type of the predicate and
>> > > > > the
>> > > > > type of the selected values. One of them is insufficient.
>> > > > >
>> > > >
>> > > >
>> > > >
>> > > > --
>> > > > Hal Finkel
>> > > > Assistant Computational Scientist
>> > > > Leadership Computing Facility
>> > > > Argonne National Laboratory
>> > > >
>> > > >
>> > >
>> > > --
>> > > Hal Finkel
>> > > Assistant Computational Scientist
>> > > Leadership Computing Facility
>> > > Argonne National Laboratory
>> > >
>> > >
>> >
>> > --
>> > Hal Finkel
>> > Assistant Computational Scientist
>> > Leadership Computing Facility
>> > Argonne National Laboratory
>> >
>> >
>> >
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141006/0d93b215/attachment.html>