Remove possible loop creation in DAGCombiner

[Hal Finkel]

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