RE: [RFC PATCH] i386: Add combine patterns to fold masked loads into binary ops [PR target/123997]

Rather than describe what I want, let me show it.  Below is a two-pattern
demo I built and tested on trunk.  It is FP add only -- deliberately
minimal, not a patch -- but it demonstrates the structure I would like v2
to follow: keep UNSPEC_MASKLOAD, no target hook, no op-specific pattern
explosion.

The key point is that you do not need to strip the UNSPEC to fold the
memory operand.  Combine already hands recog the folded form; all that is
missing is a define_insn of that shape.

1. The form the PR needs.
--------------------------

The MEM sits *inside* the UNSPEC and carries the "m" constraint, exactly
as *<avx512>_load<mode>_mask already does.  That is what makes the RA
question disappear: LRA never sees a bare MEM in an operator slot, so
there is nothing for it to reload, and the UNSPEC still records that the
access is masked so nothing downstream can widen it.

(define_insn "*add<mode>3_maskload_fold_maskz"
  [(set (match_operand:VFH_AVX512VL 0 "register_operand" "=v")
	(vec_merge:VFH_AVX512VL
	  (plus:VFH_AVX512VL
	    (unspec:VFH_AVX512VL
	      [(match_operand:VFH_AVX512VL 1 "memory_operand" "m")]
	      UNSPEC_MASKLOAD)
	    (match_operand:VFH_AVX512VL 2 "register_operand" "v"))
	  (match_operand:VFH_AVX512VL 3 "const0_operand")
	  (match_operand:<avx512fmaskmode> 4 "register_operand" "Yk")))]
  "TARGET_AVX512F"
  "vadd<ssemodesuffix>\t{%1, %2, %0%{%4%}%{z%}|%0%{%4%}%{z%}, %2, %1}"
  [(set_attr "type" "sseadd")
   (set_attr "prefix" "evex")
   (set_attr "mode" "<MODE>")])

2. Bridging what combine actually produces.
--------------------------------------------

Combine does not hand us the flat form above.  It hands us the nested one,
with the inner vec_merge still in place, because simplify_merge_mask
refuses to strip it for a trapping MEM.  From the -1.c dump on trunk:

  Failed to match this instruction:
  (set (reg:V8DF 133)
      (vec_merge:V8DF (plus:V8DF (vec_merge:V8DF (unspec:V8DF [
                          (mem:V8DF (plus:DI (mult:DI (reg:DI 129)
                                      (const_int 8))
                                  (reg/v/f:DI 120 [ a ])) [1  S64 A64])
                      ] UNSPEC_MASKLOAD)
                  (const_vector:V8DF [(const_double 0.0) repeated x8])
                  (reg:QI 118))
              (reg:V8DF 134))
          (const_vector:V8DF [(const_double 0.0) repeated x8])
          (reg:QI 118)))

Since match_dup 3 forces both masks to be the same register, the outer
mask already zeroes the off lanes and the inner merge is redundant.  So
match that shape and split to the pattern above:

(define_insn_and_split "*add<mode>3_maskload_fold_nested"
  [(set (match_operand:VFH_AVX512VL 0 "register_operand")
	(vec_merge:VFH_AVX512VL
	  (plus:VFH_AVX512VL
	    (vec_merge:VFH_AVX512VL
	      (unspec:VFH_AVX512VL
		[(match_operand:VFH_AVX512VL 1 "memory_operand")]
		UNSPEC_MASKLOAD)
	      (match_operand:VFH_AVX512VL 2 "const0_operand")
	      (match_operand:<avx512fmaskmode> 3 "register_operand"))
	    (match_operand:VFH_AVX512VL 4 "register_operand"))
	  (match_operand:VFH_AVX512VL 5 "const0_operand")
	  (match_dup 3)))]
  "TARGET_AVX512F && ix86_pre_reload_split ()"
  "#"
  "&& 1"
  [(set (match_dup 0)
	(vec_merge:VFH_AVX512VL
	  (plus:VFH_AVX512VL
	    (unspec:VFH_AVX512VL [(match_dup 1)] UNSPEC_MASKLOAD)
	    (match_dup 4))
	  (match_dup 5)
	  (match_dup 3)))])

Note the split *keeps* the UNSPEC.  That is the whole difference from your
version: we remove the redundant inner mask, not the record that the load
is masked.

3. Result.
-----------

45 lines, no other changes.  avx512-maskload-fold-1.c:

  before:  vmovupd (%r9,%rax,8), %zmm2{%k1}{z}
           vmovupd (%r8,%rax,8), %zmm1{%k1}{z}
           vaddpd  %zmm1, %zmm2, %zmm0{%k1}{z}

  after:   vmovupd (%r9,%rax,8), %zmm0{%k1}{z}
           vaddpd  (%r8,%rax,8), %zmm0, %zmm0{%k1}{z}

pr97642-1.c still emits no k regs, and a maskz-load + maskz-add built from
intrinsics is unaffected.

And op can be generalized to other operations, similar like your masked patterns.

But for the those unmasked cases, GCC doesn't have per-lane analysis in the rtl to decide if the transformation for the unmasked patterns are valid.
Maybe in the vectorizer always generates MASKED_LOAD + COND_OP + MASKED_STORE instead MASKED_LOAD + OP + MASKED_STORE. Then the masked define_insn and define_insn_and_split can automatically matches that.



BR,
Hongtao

________________________________________
From: Sarvesh Chandra <[email protected]>
Sent: Friday, 24 July 2026 12:36:50
To: [email protected]
Cc: [email protected]; [email protected]; Liu, Hongtao; [email protected]; [email protected]; [email protected]
Subject: Re: [RFC PATCH] i386: Add combine patterns to fold masked loads into binary ops [PR target/123997]

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.