Remove possible loop creation in DAGCombiner
Hal Finkel
hfinkel at anl.gov
Mon Oct 6 18:22:08 PDT 2014
----- Original Message -----
> From: "Matt Arsenault" <Matthew.Arsenault at amd.com>
> To: "deadal nix" <deadalnix at gmail.com>, "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, October 6, 2014 7:51:55 PM
> Subject: Re: Remove possible loop creation in DAGCombiner
>
>
> On 10/06/2014 04:36 PM, deadal nix wrote:
>
>
>
> I mean that if Matt is generating the wrong instruction for this
> select on his target, it is likely that he will generate the same
> thing for other selects that may ends up with the same types as
> predicated and selected.
> In my case the instruction fails to select. I suppose I could work
> around that with a specific instruction pattern to handle the
> truncation of the mask. It would work for me just to do this
> transform if the conversion to the select mask is a no-op truncate,
> and skipping if it requires another sign extension
Right. I think the point is that, regardless of this commit, nothing guarantees the invariant your target is depending on. This may have been the most common place creating SELECT nodes that could have come from SETCC nodes of a different size, but other transformations could do the same. For example, depending on the target configuration, it seems like the code that calls DAG.getSelect inside of DAGCombiner::SimplifySelectCC could also trigger this issue.
Furthermore, the existing code was both broken (because it could lead to an infinite loop) and wrong (because it did not properly account for the boolean-contents type). I recommend either handling this in your backend, or auditing all places in the SDAG code that create SELECT nodes and propose a new callback (there actually are not that many) along with a documentation update for ISDOpcodes.h noting the new constraint on the condition's type.
Thanks again,
Hal
>
> -Matt
>
>
>
>
>
>
> 2014-10-06 16:16 GMT-07:00 Hal Finkel < hfinkel at anl.gov > :
>
>
> ----- Original Message -----
> > From: "deadal nix" < deadalnix at gmail.com >
> > To: "Matt Arsenault" < arsenm2 at gmail.com >
> > Cc: "Hal Finkel" < hfinkel at anl.gov >, "llvm-commits" <
> > llvm-commits at cs.uiuc.edu >, "Nadav Rotem" < nrotem at apple.com >,
> > "Owen Anderson" < resistor at mac.com >
> > Sent: Monday, October 6, 2014 6:05:26 PM
> > Subject: Re: Remove possible loop creation in DAGCombiner
> >
> >
> >
> >
> > 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.
>
> We've added single-use callbacks before, I don't think it is a big
> deal. Do you mean there would be multiple callsites to change, or
> just this one? I thought you had asserted it would just be this one
> callsite?
>
> And even if it is just this one callsite, do we still have a problem
> if the callback requests a sign extension?
>
> -Hal
>
>
>
> > - 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
> >
> >
> >
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
>
>
> _______________________________________________
> llvm-commits mailing list llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list