On Wed, Aug 28, 2019 at 05:26:55PM -0400, Michael Meissner wrote: > On Wed, Aug 28, 2019 at 12:14:58PM -0500, Segher Boessenkool wrote: > > > +/* Enumeration giving the type of traditional addressing that would be > > > used to > > > + decide whether an instruction uses prefixed memory or not. If the > > > + traditional instruction uses the DS instruction format, and the > > > bottom 2 > > > + bits of the offset are not 0, the traditional instruction cannot be > > > used, > > > + but a prefixed instruction can be used. */ > > > > "Traditional" is a bad word for documentation. What you mean is what was > > supported before. Before you know it "new" will be old as well. > > Yeah, yeah, yeah. I recall in Amsterdam there is the "Oude Kerk" (old church) > built in the 1200's and the "De Nieuwe Kerk" in Amsterdam (built in the > 1500's) > and thinking then of the problems of calling something "new" and "old".
:-) > > Can you fix this struct / arrays / whatever, instead of adding more to it? > > And these "address masks" are bitmaps of random flags, one for each > > "register class" (which is not related to the core GCC concept of "register > > class", and the bits are called "RELOAD_REG_*" although this isn't for > > reload at all? > > Actually no, they were created explicitly for the secondary reload handler > when > I wrote this interface to add VSX support. This is not just for reload anymore, so please don't name it that. Renaming things isn't hard, this isn't a public API or anything :-) > > > + if ((addr_mask & quad_flags) == RELOAD_REG_OFFSET > > > + && ((rc == RELOAD_REG_GPR && msize >= 8 && TARGET_POWERPC64) > > > + || (rc == RELOAD_REG_VMX))) > > > + addr_mask |= RELOAD_REG_DS_OFFSET; > > > + > > > reg_addr[m].addr_mask[rc] = addr_mask; > > > - any_addr_mask |= addr_mask; > > > + any_addr_mask |= (addr_mask & ~RELOAD_REG_AND_M16); > > > > Why do you need this last line? Why was that flag set at all? What does > > "any mask" mean if it is not? > > The flag is set to say this register class allows the funky (reg + reg) & -16 > addressing used with the original Altivec instructions. No, I understand that, but why was it set in some individual mask if you need to clean it in the "any" mask? > > > @@ -10770,11 +10855,10 @@ rs6000_secondary_reload_memory (rtx addr > > > & ~RELOAD_REG_AND_M16); > > > > > > /* If the register allocator hasn't made up its mind yet on the > > > register > > > - class to use, settle on defaults to use. */ > > > + class to use, use the default address mask bits. */ > > > else if (rclass == NO_REGS) > > > > And this *does* mean register class. > > No, in the context of the code, it means reload register class. rclass is a register class. NO_REGS is a register class. "rc" isn't. > The whole > point is to reduce all of the normal register classes just to the 3 hardware > register types. Yes, so don't call it register class. Don't use the same word for two different things, esp. when one is used all over the place already. > > I think this would all be much simpler with just a few lines of code instead > > of all these tables, fwiw. That's the core of most of this. All this precomputation is indirection that makes things really hard to understand. And a lot of the more problematic code is the *older* code. If you improve that first -- *first*, that is what the earlier patches in a series are for -- then this all will be much much easier to read and understand and review and comment on and accept. > > > +;; Load up a pc-relative address. Print_operand_address will append a > > > @pcrel > > > +;; to the symbol or label. > > > +(define_insn "pcrel_local_addr" > > > > This isn't used anywhere? Not by name, that is? > > Yes it is used in rs6000_emit_move. Not in this patch though? > > > + [(set (match_operand:DI 0 "gpc_reg_operand" "=b*r") > > > + (match_operand:DI 1 "pcrel_local_address"))] > > > + "TARGET_PCREL" > > > + "pla %0,%a1" > > > + [(set_attr "length" "12")]) > > > > I wonder if that whole "b*r" thing is useful at all these days, btw. > > Yep. You mean it is useful? Or you question it too? > > This patch changes a whole bunch of things. You probably can split it > > into smaller, self-contained pieces. > > Not really, but I will try. However, then of course you have the issue that a > particular patch creates a function that isn't used for a few patches, and you > have to look at several patches all at once. No, not if you divide things properly. You *never* need to introduce more than one thing at once, if they all are unused! Multiple concepts in one patch is a LOT of work to review. It is MUCH less work to review 50 focused patches than to review just 5 doing the same, even if those 50 make up twice as many lines of patch total. Segher
