Re: RFC: Allow "(mem:<vectype> (reg:<vectype>))"
Andrew Stubbs <[email protected]> Fri, 3 Jul 2026 12:43:02 +0100
| Newsgroups | gmane.comp.gcc.devel |
|---|---|
| Message-ID | <[email protected]> |
On 25/06/2026 18:22, Andrew Stubbs wrote: > On 25/06/2026 12:22, Richard Biener wrote: >> On Thu, Jun 25, 2026 at 12:20 PM Andrew Stubbs <[email protected]> wrote: >>> >>> On 25/06/2026 10:05, Richard Biener wrote: >>>> On Thu, Jun 25, 2026 at 10:52 AM Andrew Stubbs <[email protected]> >>>> wrote: >>>>> >>>>> On 23/06/2026 13:28, Michael Matz wrote: >>>>>> Hello, >>>>>> >>>>>> On Mon, 22 Jun 2026, Andrew Stubbs wrote: >>>>>> >>>>>>>> TARGET_POINTER_MODE and TARGET_ADDR_SPACE_POINTER mode only >>>>>>>> return one mode, >>>>>>>> but now we have multiple valid modes: DImode, V2DImode, >>>>>>>> V4DImode, etc. They >>>>>>>> also return scalar_int_mode so I can't even invent a new address >>>>>>>> space for >>>>>>>> the vector pointers. >>>>>>>> >>>>>>>> Same for TARGET_ADDR_SPACE_ADDRESS_MODE. >>>>>>>> >>>>>>>> TARGET_VALID_POINTER_MODE and >>>>>>>> TARGET_ADDR_SPACE_VALID_POINTER_MODE only >>>>>>>> accept scalar_int_mode, so automatically exclude vector pointers. >>>>>>>> >>>>>>>> These can be patched to use plain machine_mode, perhaps, or keep >>>>>>>> them all >>>>>>>> scalar and add a new TARGET_ADDR_SPACE_ALLOWS_VECTOR_POINTERS to >>>>>>>> fix the >>>>>>>> problem at the call sites, which might make more sense? >>>>>>> >>>>>>> I've been working on this proposal some more. I've solved the >>>>>>> above problem by >>>>>>> keeping all the hooks returning pointer/address types scalar, but >>>>>>> changing >>>>>>> TARGET_ADDR_SPACE_VALID_POINTER_MODE to accept vector types, so >>>>>>> the backend >>>>>>> can approve them. When the target independent code encounters a >>>>>>> vector of >>>>>>> addresses it checks that the inner mode matches the appropriate >>>>>>> hook values. >>>>>>> >>>>>>> I was hoping to post a patch series this week, but I've hit >>>>>>> against another >>>>>>> issue..... >>>>>>> >>>>>>> I have an insn that looks like this: >>>>>>> >>>>>>> (set (reg:V64SI 123) >>>>>>> (vec_merge:V64SI >>>>>>> (mem:V64SI (reg:V64DI 456)) >>>>>>> (reg:V64SI 123) >>>>>>> (reg:DI 789))) >>>>>> >>>>>> How would anything in RTL-land see that this load is masked? >>>>>> There's no >>>>>> info anywhere that makes it differ from >>>>>> >>>>>> (set (reg:V64SI 123) >>>>>> (vec_merge:V64SI >>>>>> (mem:V64SI (reg:V64DI 456)) >>>>>> (reg:V64SI 123) >>>>>> (reg:DI 789))) >>>>>> >>>>>> with me saying that this is a vector merge operation with a memory >>>>>> operand >>>>>> that is loaded completely. It's a verbatim copy of your RTL >>>>>> pattern. See >>>>>> my point? :) >>>>> >>>>> I'm pretty sure you just restated my point. ;-) >>>>> >>>>>>> I can workaround the "reload" in the backend by using unspecs >>>>>>> (exactly >>>>>>> what I was trying to avoid), but does anyone have a better >>>>>>> suggestion? >>>>>> >>>>>> (a) unspec >>>>>> (b) teaching RTL-land generally that vec_merge(...(mem)) is >>>>>> "special" and >>>>>> its operands cannot just be lifted out (nah) >>>>>> (c) biting the bullet and introduce and set a flag on (mem)s that >>>>>> it is >>>>>> "special"/partial >>>>>> (c') similar flag on the vec_merge >>>>>> (d) biting a different bullet and introduce a new top-level RTL >>>>>> expression >>>>>> (partial_mem:mode (addr) (mask)) (with either target- >>>>>> defined methods >>>>>> to specify content of unselected bits/bytes, or with a >>>>>> third operand >>>>>> to specify them (which then makes this just a different >>>>>> vec_merge) >>>>>> >>>>>> I think (d) is most elegant and most involved, (c) or (c') are the >>>>>> "best" >>>>>> on a cost/benefit basis, (b) the most hacky, but may be fine if >>>>>> done via a >>>>>> target hook and (a) the easiest but most unelegant. (a) has >>>>>> subcases: >>>>>> where to put the unspec: around the (mem), around the (vec_merge), >>>>>> around >>>>>> the whole (set). IMHO, around the vec_merge makes "most sense", >>>>>> whatever >>>>>> that means for unspecs, but, well, still unspecs :-/ >>>>> >>>>> I've been working to implement an unspec solution in the backend, for >>>>> now, so that I can move on with this and get the actually working bits >>>>> proposed. >>>>> >>>>> Putting an unspec around the whole pattern doesn't help. Nor does >>>>> replacing the vec_merge with an unspec. In both cases the register >>>>> allocator happily tries to break out the MEM, just as before. >>>>> >>>>> I tried creating an unspec that matches a custom memory constraint >>>>> (the >>>>> idea being that it will drop into the existing vec_merge patterns), >>>>> but >>>>> the register allocator now breaks out the unspec into another >>>>> step. If >>>>> I use a non-memory-specific constraint then the same thing >>>>> happens. If >>>>> I place the mask inside the unspec and have a specific instruction >>>>> handle that, then it works, but now I have a masked load feeding >>>>> into a >>>>> masked move for no good reason. >>>>> >>>>> Here's what it looks like now: >>>>> >>>>> (set (reg:V64SI 123) >>>>> (vec_merge:V64SI >>>>> (mem:V64SI (unspec:V64DI [(reg:V64DI 456) (reg:DI 789)] >>>>> UNSPEC_PARTIAL_ADDRVEC)) >>>>> (reg:V64SI 123) >>>>> (reg:DI 789))) >>>>> >>>>> Note that the mask register, r789, now also appears within the MEM. >>>>> When >>>>> reload tries to break it out, gen_movv64si sees the new unspec and >>>>> expands to a masked load. The new instruction is basically >>>>> identical to >>>>> the one that it came from, but reload seems happy now, and my program >>>>> runs correctly without the memory fault. >>>> >>>> x86 gets away with >>>> >>>> (define_insn "*<avx512>_load<mode>_mask" >>>> [(set (match_operand:V48_AVX512VL 0 "register_operand" "=v") >>>> (vec_merge:V48_AVX512VL >>>> (unspec:V48_AVX512VL >>>> [(match_operand:V48_AVX512VL 1 "memory_operand" "m")] >>>> UNSPEC_MASKLOAD) >>>> (match_operand:V48_AVX512VL 2 "nonimm_or_0_operand" "0C") >>>> (match_operand:<avx512fmaskmode> 3 "register_operand" "Yk")))] >>>> >>>> so the (unspec ...) is wrapped around the MEM. >>> >>> I tried that; it just tried to reload the entire unspec. >> >> Ah... aarch64 has >> >> (define_insn "*aarch64_maskload<mode><vpred>" >> [(set (match_operand:SVE_ALL 0 "register_operand" "=w") >> (unspec:SVE_ALL >> [(match_operand:<VPRED> 2 "register_operand" "Upl") >> (match_operand:SVE_ALL 1 "memory_operand" "m") >> (match_operand:SVE_ALL 3 "aarch64_maskload_else_operand")] >> UNSPEC_LD1_SVE))] >> "TARGET_SVE" >> "ld1<Vesize>\t%0.<Vctype>, %2/z, %1" >> [(set_attr "sve_type" "sve_load_1reg")] >> ) >> >> so they wrap the whole SET_RHS. >> >> I think we should document some canonical way to represent the >> masking side-effect given there's no native RTL construct. > > Again, I think this one only works if operand 1 never gets reloaded, > which may be OK as there's no early clobber. > > I had to modify my scheme though; it turns out (mem (unspec)) doesn't > permit LRA to see the base/offset registers, so it only works if the > registers within happen to get assigned to the right classes by other > instructions. However, there's provision in there to "add" a machine- > specific unspec (known as "segment" in rtlanal.cc), so I can use that. > My addresses now look like this: > > (mem:V64SI (plus:V64DI > (unspec:V64DI [(reg:DI mask_reg)] UNSPEC_MASK) > (plus:V64DI > (reg:V64DI base_reg) > (const_int offset)))) > > The mask_reg is only ever used to force new load instructions to use the > vec_merge pattern, when something (a reload) tries to use the MEM out of > context. I had to give up on this scheme. It solved the immediate problem quite neatly, but the extra "mask_reg" element is causing reloads by itself. There's no way to specify that it should match the mask operand of vec_merge (or indeed add any constraint at all), so it was getting assigned to an independent register. Sometimes this was fine, but sometimes it made a mess of the codegen. I've now got a specific insn for masked loads that has bare address, offset, and address space operands (no explicit MEM). This isn't what I wanted, but I'm still using vector mem for the unmasked loads and all stores. Andrew