> Am 05.07.2024 um 16:00 schrieb Richard Sandiford <richard.sandif...@arm.com>:
>
> 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.
Agreed. I also think the predicate idea for the else value in MASK_LOAD is
good.
Richard
> Thanks,
> Richard
