Thanks for the review, and for the detailed comments. Replies inline.
For readability I have omitted the actual patch from this mail; I will
revise it based on the review received.
> > +;; Return true for non-commutative binary operators (minus, div).
> > +(define_predicate "noncommutative_binary_operator"
> > + (match_code "minus,div"))
> The name is misleading, since we only have minus, div, better to be
> named to sub_div_operator?
Okay, renamed to sub_div_operator.
> > + rtx vm = gen_rtx_VEC_MERGE (<MODE>mode, op, operands[3], operands[4]);
> It should be equal to vec_merge (op (operands[2], operands[5]),
> op (0, operands[5]), mask), not vec_merge (op (operands[2], operands[5]),
> 0, mask). op (0, operands[5]) is not always equal to 0. .i.e when op is
> add, op (0, operands[5]) is operands[5]. Similar for
> noncomm_maskload_fold_unmasked_op.
You are right, the two unmasked patterns are wrong in the general case:
the off-lane value is op (0, operands[5]), which is only 0 when
op (0, x) == 0 (mult, and, umin), not for add/ior/xor/etc. The bug is
directly observable with intrinsics (a maskz load feeding a plain
unmasked op whose full result is then read, e.g. _mm512_maskz_loadu_epi32
(k, a) followed by _mm512_add_epi32 and a full store, has off lanes
0 + b, which the forced-0 fold clobbers to 0).
To emit the correct off-lane value, op (0, operands[5]) has to go in the
merge source of the vec_merge. The existing vec_merge (op ...) masked
patterns already provide both a zero-masked source (const0) and a
merge-masked source (the destination register), so two cases are covered
with no new define_insns: op (0, x) == 0 (mult, and, umin) uses the
zero-masked form, and op (0, x) == operands[5] (add, ior, xor) uses the
merge-masked form with operands[5] as the destination/merge source. The
remaining operators need something that is neither const0 nor
operands[5], for sub it is neg (operands[5]), for div it is
0/operands[5], and so on, none of which the current masked patterns
accept, so each would need its own define_insn, i.e. the pattern
explosion we were trying to avoid. I tried emitting op (0, operands[5])
directly in the splitter for these and it ICEs in IRA (unrecognizable
insn), since the masked op's merge source can only be const0 or a
register, not another operation. We could materialise op (0,
operands[5]) into a register first, but that reintroduces the extra
vector value we were folding away, defeating the purpose of the patch.
So rather than encode op (0, operands[5]) per operator, I would prefer
to drop all four patterns and use a generic hoist-to-root transform in
combine (using a target hook): pull the (possibly nested, as in
vpavgb/w) vec_merge out to the root and apply it once, gated on (a) all
operands zero-masked with the same mask and (b) op (all operands 0) ==
0. Condition (b) is checked with the existing simplify-rtx folding
(arity from GET_RTX_LENGTH, simplify_binary_operation /
simplify_ternary_operation over CONST0_RTX), so it stays arity-generic
and correct for every operator, and also covers the vpavgb/w case from
the RFC note. (I did also check whether the masked-epilogue origin
makes these off lanes dead in practice; that turns out to be true but
not something combine can rely on, so I have moved the details to the
end of this mail.)
> But if we remove UNSPEC, splitter it before RA, RA may reload the whole
> memory to a register? It may issue an segment faullt when the whole
> memory is invalid?
I looked into this, and RA does not reload the memory in this case.
Each of the target patterns that recognises the folded RTL vec_merge (op
(mem, reg), 0, k) has the memory operand in the same alternative as the
register (v/x), so memory is a legal match for that operand. The op is
commutative, so LRA scores two operand orderings. In the ordering it
picks, the folded a[i] stays as the MEM in op2 (the memory-accepting
vmBr slot): losers=0, overall=2, reload_nregs=0. The only other
ordering it tries is the commutative swap, which puts the memory operand
into op1 (register-only, v) and so would have to reload the MEM into a
register (Operand reload: losers++): losers=1, overall=9, refused. The
decision is made on the first key (losers), so the MEM-keeping ordering
wins and the mem is left in place; the overall=2 there is just the
dst==src1 matching-tie penalty, not a memory penalty.
This works because the split output matches a pre-existing constrained
insn: the integer add lands on *<insn><mode>3_mask (op2 vm), the FP
add/sub/mul form on *<insn><mode>3<mask_name><round_name> (op2
xBm,vmBr). In every one of these the memory constraint sits in the same
alternative as v, so memory is always a legal match and RA leaves the
mem embedded. Every op we target (comm and sub/div) already has such a
memory-accepting masked pattern. Per the Intel SDM EVEX suppresses
memory faults on the masked-off lanes for these ops, so removing the
UNSPEC and letting vec_merge (op (mem)) keep the mem embedded is safe
at RA time. We would like to confirm whether this is indeed safe,
given the above.
> so maybe it's better to be a post_reload splitter or we just add
> define_insn for all of those with some magic of subst.
Given the above, a post-RA splitter should not be needed. The subst
route would be to change the combiner so it places the vec_merge on the
outside with the UNSPEC still inside, and then use subst to derive UNSPEC
variants of the existing *<insn><mode>3<mask_name><round_name> etc.
patterns, so reload sees the constraints. Or we could have a constrained
define_insn_and_split that directly emits folded and masked assembly.
Either way this is back to op-specific patterns and needs more variants,
which is what the combine transform above avoids, so I would lean towards
the combine transform unless you see a problem with it.
What do you think?
Regards,
Sarvesh
---
Additional experiment: are the off lanes dead in the vectorizer case?
I also checked whether the masked-epilogue origin (PR123997) makes the
off lanes dead, so that forcing 0 would be harmless. It does, but not
because op (0, operands[5]) is 0 (it often is not, e.g. a[i] + s gives an
off lane of 0 + s = s). It is dead because of how the vectorizer emits
the op:
- If the op is masked (mask_out_inactive in vectorizable_operation: it may
trap or is a reduction), the off lanes are handled by the loop mask
already, so there is no separate off-lane question.
- If the op is left unmasked, its result can only leave the loop two ways,
and both are gated by the loop mask, so the off lanes are never
observed:
* through memory: in a fully-masked loop vectorizable_store has no
unmasked-store path, it always emits MASK_STORE, so off lanes are
not written.
* as a live-out value: vectorizable_live_operation extracts the last
active lane (IFN_EXTRACT_LAST, or a BIT_FIELD_REF of the final lane
when the cost model picks full-width plus tapered epilogues), never
an off lane.
I confirmed this on trunk: for a[i] + s the op off lane is s (nonzero) yet
every consumer is a MASK_STORE or a last-active-lane extract, and any
construct that would read an off lane live (e.g. a reversing store) makes
the cost model drop partial vectors entirely and use full loads. So the
deadness is a whole-loop property that combine cannot see; the fold must
not rely on it, which is why the hoist-to-root transform above still
checks op (all operands 0) == 0 rather than assuming the off lanes are
dead.
