This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.
> 
> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]