This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH V3, #1 of 10], Add basic pc-relative support
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.
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.