Remove possible loop creation in DAGCombiner
Matt Arsenault
Matthew.Arsenault at amd.com
Mon Oct 6 17:51:55 PDT 2014
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
-Matt
>
> 2014-10-06 16:16 GMT-07:00 Hal Finkel <hfinkel at anl.gov
> <mailto:hfinkel at anl.gov>>:
>
> ----- Original Message -----
> > From: "deadal nix" <deadalnix at gmail.com
> <mailto:deadalnix at gmail.com>>
> > To: "Matt Arsenault" <arsenm2 at gmail.com <mailto:arsenm2 at gmail.com>>
> > Cc: "Hal Finkel" <hfinkel at anl.gov <mailto:hfinkel at anl.gov>>,
> "llvm-commits" <llvm-commits at cs.uiuc.edu
> <mailto:llvm-commits at cs.uiuc.edu>>, "Nadav Rotem"
> <nrotem at apple.com <mailto:nrotem at apple.com>>,
> > "Owen Anderson" <resistor at mac.com <mailto: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
> <mailto:arsenm2 at gmail.com> > :
> >
> >
> >
> >
> >
> >
> > On Oct 5, 2014, at 10:29 PM, deadal nix < deadalnix at gmail.com
> <mailto: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
> <mailto:hfinkel at anl.gov> > :
> >
> >
> > ----- Original Message -----
> > > From: "deadal nix" < deadalnix at gmail.com
> <mailto:deadalnix at gmail.com> >
> > > To: "Hal Finkel" < hfinkel at anl.gov <mailto:hfinkel at anl.gov> >
> > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu
> <mailto:llvm-commits at cs.uiuc.edu> >, "Nadav Rotem" <
> > > nrotem at apple.com <mailto:nrotem at apple.com> >, "Owen Anderson"
> < resistor at mac.com <mailto:resistor at mac.com> >,
> > > "Matt Arsenault" < arsenm2 at gmail.com <mailto: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
> <mailto: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
> <mailto:hfinkel at anl.gov> > :
> > >
> > >
> > >
> > >
> > > ----- Original Message -----
> > > > From: "deadal nix" < deadalnix at gmail.com
> <mailto:deadalnix at gmail.com> >
> > > > To: "Hal Finkel" < hfinkel at anl.gov <mailto:hfinkel at anl.gov> >
> > > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu
> <mailto:llvm-commits at cs.uiuc.edu> >, "Nadav Rotem" <
> > > > nrotem at apple.com <mailto:nrotem at apple.com> >, "Owen
> Anderson" < resistor at mac.com <mailto:resistor at mac.com> >,
> > > > "Matt Arsenault" < arsenm2 at gmail.com
> <mailto: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
> <mailto:hfinkel at anl.gov> > :
> > > >
> > > >
> > > > ----- Original Message -----
> > > > > From: "deadal nix" < deadalnix at gmail.com
> <mailto:deadalnix at gmail.com> >
> > > > > To: "Hal Finkel" < hfinkel at anl.gov <mailto:hfinkel at anl.gov> >
> > > > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu
> <mailto:llvm-commits at cs.uiuc.edu> >, "Nadav Rotem"
> > > > > <
> > > > > nrotem at apple.com <mailto:nrotem at apple.com> >, "Owen
> Anderson" < resistor at mac.com <mailto:resistor at mac.com> >,
> > > > > "Matt Arsenault" < arsenm2 at gmail.com
> <mailto: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
> <mailto:hfinkel at anl.gov> > :
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > ----- Original Message -----
> > > > > > From: "deadal nix" < deadalnix at gmail.com
> <mailto:deadalnix at gmail.com> >
> > > > > > To: "Hal Finkel" < hfinkel at anl.gov
> <mailto:hfinkel at anl.gov> >
> > > > >
> > > > >
> > > > > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu
> <mailto:llvm-commits at cs.uiuc.edu> >, "Nadav
> > > > > > Rotem"
> > > > > > <
> > > > > > nrotem at apple.com <mailto:nrotem at apple.com> >, "Owen
> Anderson" < resistor at mac.com <mailto:resistor at mac.com> >,
> > > > > > "Matt Arsenault" < arsenm2 at gmail.com
> <mailto: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
> <mailto:hfinkel at anl.gov> > :
> > > > > >
> > > > > >
> > > > > > ----- Original Message -----
> > > > > > > From: "deadal nix" < deadalnix at gmail.com
> <mailto:deadalnix at gmail.com> >
> > > > > > > To: "Hal Finkel" < hfinkel at anl.gov
> <mailto:hfinkel at anl.gov> >
> > > > > > > Cc: "llvm-commits" < llvm-commits at cs.uiuc.edu
> <mailto:llvm-commits at cs.uiuc.edu> >, "Nadav
> > > > > > > Rotem"
> > > > > > > <
> > > > > > > nrotem at apple.com <mailto:nrotem at apple.com> >, "Owen
> Anderson" < resistor at mac.com <mailto: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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141006/a376ab22/attachment.html>
More information about the llvm-commits
mailing list