Remove possible loop creation in DAGCombiner

Wed Oct 1 12:13:29 PDT 2014

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141001/8c748bc4/attachment.html>