On Mon, Sep 3, 2012 at 3:39 PM, Marc Glisse <marc.gli...@inria.fr> wrote:
> On Mon, 3 Sep 2012, Richard Guenther wrote:
>
>> Please do the early outs where you compute the arguments. Thus, right
>> after getting op0 in this case or right after computing n for the n != 1
>> check.
>
>
> Ok.
>
>> I think you need to verify that the type of 'op' is actually the element
>> type
>> of op0. The BIT_FIELD_REF can happily access elements two and three
>> of { 1, 2, 3, 4 } as a long for example.
>
>
> Indeed I missed that.
>
>
>> See the BIT_FIELD_REF foldings in fold-const.c.
>
>
> That's what I was looking at (picked the same variable names size, idx, n)
> but I forgot that test :-(
>
>
>>> + if (code == VEC_PERM_EXPR)
>>> + {
>>> + tree p, m, index, tem;
>>> + unsigned nelts;
>>> + m = gimple_assign_rhs3 (def_stmt);
>>> + if (TREE_CODE (m) != VECTOR_CST)
>>> + return false;
>>> + nelts = VECTOR_CST_NELTS (m);
>>> + idx = TREE_INT_CST_LOW (VECTOR_CST_ELT (m, idx));
>>> + idx %= 2 * nelts;
>>> + if (idx < nelts)
>>> + {
>>> + p = gimple_assign_rhs1 (def_stmt);
>>> + }
>>> + else
>>> + {
>>> + p = gimple_assign_rhs2 (def_stmt);
>>> + idx -= nelts;
>>> + }
>>> + index = build_int_cst (TREE_TYPE (TREE_TYPE (m)), idx * size);
>>> + tem = fold_build3 (BIT_FIELD_REF, TREE_TYPE (op), p, op1, index);
>>
>>
>> This shouldn't simplify, so you can use build3 instead.
>
>
> I think that it is possible for p to be a VECTOR_CST, if the shuffle
> involves one constant and one non-constant vectors, no?
Well, constant propagation should have handled it ...
If you use fold_build3 you need to check that the result is in expected form
(a is_gimple_invariant or an SSA_NAME).
> Now that I look at this line, I wonder if I am missing some unshare_expr for
> p and/or op1.
If either is a CONSTRUCTOR and its def stmt is not removed and it survives
into tem then yes ...
>> Please also add handling of code == CONSTRUCTOR.
>
>
> The cases I tried were already handled by fre1. I can add code for
> constructor, but I'll need to look for a testcase first. Can that go to a
> different patch?
Yes.
Thanks,
Richard.
> --
> Marc Glisse
