Richard Biener <rguent...@suse.de> writes:
> On Fri, 5 Jul 2024, Richard Biener wrote:
>
>> On Fri, 5 Jul 2024, Robin Dapp wrote:
>>
>> > Hi,
>> >
>> > in PR115336 we have the following
>> >
>> > vect_patt_391 = .MASK_LEN_GATHER_LOAD (_470, vect__59, 1, { 0, ... }, {
>> > 0, ... }, _482, 0);
>> > vect_iftmp.44 = vect_patt_391 | { 1, ... };
>> > .MASK_LEN_STORE (vectp_f.45, 8B, { -1, ... }, _482, 0, vect_iftmp.44);
>> >
>> > which assumes that a maskload sets the masked-out elements to zero.
>>
>> To me this looks like mis-applying of match.pd:6083?
>>
>> Applying pattern match.pd:6083, gimple-match-1.cc:45749
>> gimple_simplified to iftmp.0_62 = iftmp.0_61 | _219;
>> new phi replacement stmt
>> iftmp.0_62 = iftmp.0_61 | _219;
>>
>> so originally it wasn't
>>
>> iftmp.0_61 = .MASK_LOAD (_260, 8B, _259);
>> iftmp.0_62 = iftmp.0_61 | _219;
>>
>> but sth like
>>
>> iftmp.0_62 = <mask> ? _219 : iftmp.0_61;
>>
>> and iftmp.0_61 is zero-one-valued because it is _Bool. Shouldn't
>> if-conversion always turn the COND_EXPR into a .COND_IOR here?
>>
>> > RVV does not guarantee this and, unfortunately, leaves it to the
>> > hardware implementation to do basically anything it wants (even keep
>> > the previous value). In the PR this leads to us reusing a previous
>> > vector register and stale, nonzero values causing an invalid result.
>> >
>> > Based on a Richard Sandiford's feedback I started with the attached
>> > patch - it's more an RFC in its current shape and obviously not
>> > tested exhaustively.
>> >
>> > The idea is:
>> > - In ifcvt emit a VCOND_MASK (mask, load_result, preferred_else_value)
>> > after a MASK_LOAD if the target requires it.
>> > - Elide the VCOND_MASK when there is a COND_OP with a replacing else
>> > value later.
>> >
>> > Is this, generally, reasonable or is there a better way?
>> >
>> > My initial idea was to add an else value to MASK_LOAD. Richard's
>> > concern was, though, that this might make nonzero else values
>> > appear inexpensive when they are actually not.
>
> In general I'd agree that we want an else value for .MASK_LOAD
> and also possibly for .MASK_STORE (but then we need to represent an
> else value that says do-not-change/merge).
Not sure about MASK_STORE. AFAICT, an else value for MASK_STORE would
be directly equivalent to:
tmp = vec_cond_expr <mask, store_value, else_value>;
*ptr = tmp;
and IMO it'd be better to keep representing it like that instead.
That is, MASK_STORE is changing something that already has a definition
(the target memory), and so there's no ambiguity in having a masked
operation that only changes part of the target.
But for MASK_LOAD and other masked vector-producing functions, we're
creating a new full vector based on a partial vector operation.
There's no existing target to modify, and so we need to get the
definition of the inactive elements from somewhere else.
Thanks,
Richard
