| Message ID | 20190628001235.GA25087@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | PowerPC Prefixed Memory, Patch #3, Update predicates | expand |
Hi Mike, On Thu, Jun 27, 2019 at 08:12:35PM -0400, Michael Meissner wrote: > + return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op) > + && SYMBOL_REF_LOCAL_P (op)); Please break the line before that first && as well? > +(define_predicate "pcrel_external_address" > + (match_code "symbol_ref,const") > +{ > + if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM) > + return false; if (!(TARGET_PCREL && TARGET_CMODEL == CMODEL_MEDIUM)) Should there be a helper function for this? PCREL is only supported for medium model -- abstracting that makes both the current code easier to read, and if we ever want to support other models, that will be simpler to do as well. > + /* Discard any CONST's. */ > + if (GET_CODE (op) == CONST) > + op = XEXP (op, 0); That comment says nothing the code already tells. It's a common thing to do anyway; just don't comment on it? > +;; Return 1 if op is a memory operand to an external variable when we > +;; support pc-relative addressing and the PCREL_OPT relocation to > +;; optimize references to it. > +(define_predicate "pcrel_external_mem_operand" > + (match_code "mem") > +{ > + return pcrel_external_address (XEXP (op, 0), Pmode); > +}) Is that comment correct? pcrel_external_address does nothing with PCREL_OPT? Or _its_ comment doesn't say, at least. Okay for trunk with those trivialities resolved. Thanks! Segher
On 6/28/19 8:20 AM, Segher Boessenkool wrote: > Hi Mike, > > On Thu, Jun 27, 2019 at 08:12:35PM -0400, Michael Meissner wrote: >> + return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op) >> + && SYMBOL_REF_LOCAL_P (op)); > Please break the line before that first && as well? > >> +(define_predicate "pcrel_external_address" >> + (match_code "symbol_ref,const") >> +{ >> + if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM) >> + return false; > if (!(TARGET_PCREL && TARGET_CMODEL == CMODEL_MEDIUM)) > > Should there be a helper function for this? PCREL is only supported > for medium model -- abstracting that makes both the current code easier > to read, and if we ever want to support other models, that will be > simpler to do as well. I'd suggest if (!rs6000_pcrel_p ()) which will also make sure this is ELFv2. Thanks, Bill > >> + /* Discard any CONST's. */ >> + if (GET_CODE (op) == CONST) >> + op = XEXP (op, 0); > That comment says nothing the code already tells. It's a common thing > to do anyway; just don't comment on it? > >> +;; Return 1 if op is a memory operand to an external variable when we >> +;; support pc-relative addressing and the PCREL_OPT relocation to >> +;; optimize references to it. >> +(define_predicate "pcrel_external_mem_operand" >> + (match_code "mem") >> +{ >> + return pcrel_external_address (XEXP (op, 0), Pmode); >> +}) > Is that comment correct? pcrel_external_address does nothing with > PCREL_OPT? Or _its_ comment doesn't say, at least. > > Okay for trunk with those trivialities resolved. Thanks! > > > Segher >
On Fri, Jun 28, 2019 at 08:20:35AM -0500, Segher Boessenkool wrote: > Hi Mike, > > On Thu, Jun 27, 2019 at 08:12:35PM -0400, Michael Meissner wrote: > > + return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op) > > + && SYMBOL_REF_LOCAL_P (op)); > > Please break the line before that first && as well? Ok. > > +(define_predicate "pcrel_external_address" > > + (match_code "symbol_ref,const") > > +{ > > + if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM) > > + return false; > > if (!(TARGET_PCREL && TARGET_CMODEL == CMODEL_MEDIUM)) > > Should there be a helper function for this? PCREL is only supported > for medium model -- abstracting that makes both the current code easier > to read, and if we ever want to support other models, that will be > simpler to do as well. Yeah, Bill's suggestion is probably what I should use. > > + /* Discard any CONST's. */ > > + if (GET_CODE (op) == CONST) > > + op = XEXP (op, 0); > > That comment says nothing the code already tells. It's a common thing > to do anyway; just don't comment on it? Ok. > > +;; Return 1 if op is a memory operand to an external variable when we > > +;; support pc-relative addressing and the PCREL_OPT relocation to > > +;; optimize references to it. > > +(define_predicate "pcrel_external_mem_operand" > > + (match_code "mem") > > +{ > > + return pcrel_external_address (XEXP (op, 0), Pmode); > > +}) > > Is that comment correct? pcrel_external_address does nothing with > PCREL_OPT? Or _its_ comment doesn't say, at least. Yes, I will re-word it. We will need this predicate even if PCREL_OPT is not being used. > Okay for trunk with those trivialities resolved. Thanks! > > > Segher
Index: gcc/config/rs6000/predicates.md =================================================================== --- gcc/config/rs6000/predicates.md (revision 272766) +++ gcc/config/rs6000/predicates.md (working copy) @@ -1645,21 +1645,57 @@ (define_predicate "pcrel_address" op = op0; } - return LABEL_REF_P (op) || SYMBOL_REF_PCREL_P (op); + if (LABEL_REF_P (op)) + return true; + + return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op) + && SYMBOL_REF_LOCAL_P (op)); }) -;; Return 1 if op is a prefixed memory operand +;; Return true if the operand is an external symbol whose address can be loaded +;; into a register either via PADDI (if the symbol is in the main program) or +;; PLD GOT address (if the symbol is defined in a shared library). + +(define_predicate "pcrel_external_address" + (match_code "symbol_ref,const") +{ + if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM) + return false; + + /* Discard any CONST's. */ + if (GET_CODE (op) == CONST) + op = XEXP (op, 0); + + /* Validate offset. */ + if (GET_CODE (op) == PLUS) + { + rtx op0 = XEXP (op, 0); + rtx op1 = XEXP (op, 1); + + if (!CONST_INT_P (op1) || !SIGNED_34BIT_OFFSET_P (INTVAL (op1))) + return false; + + op = op0; + } + + return (SYMBOL_REF_P (op) && !SYMBOL_REF_LOCAL_P (op)); +}) + +;; Return 1 if op is a prefixed memory operand. (define_predicate "prefixed_mem_operand" (match_code "mem") { return rs6000_prefixed_address (XEXP (op, 0), GET_MODE (op)); }) -;; Return 1 if op is a memory operand that is not a prefixed memory -;; operand. -(define_predicate "non_prefixed_mem_operand" - (and (match_operand 0 "memory_operand") - (not (match_operand 0 "prefixed_mem_operand")))) +;; Return 1 if op is a memory operand to an external variable when we +;; support pc-relative addressing and the PCREL_OPT relocation to +;; optimize references to it. +(define_predicate "pcrel_external_mem_operand" + (match_code "mem") +{ + return pcrel_external_address (XEXP (op, 0), Pmode); +}) ;; Match the first insn (addis) in fusing the combination of addis and loads to ;; GPR registers on power8. Index: gcc/config/rs6000/rs6000.h =================================================================== --- gcc/config/rs6000/rs6000.h (revision 272766) +++ gcc/config/rs6000/rs6000.h (working copy) @@ -2550,10 +2550,3 @@ typedef struct GTY(()) machine_function IN_RANGE ((VALUE), \ -(HOST_WIDE_INT_1 << 33), \ (HOST_WIDE_INT_1 << 33) - 1 - (EXTRA)) - -/* Flag to mark SYMBOL_REF objects to say they are local addresses and are used - in pc-relative addresses. */ -#define SYMBOL_FLAG_PCREL SYMBOL_FLAG_MACH_DEP - -#define SYMBOL_REF_PCREL_P(X) \ - (SYMBOL_REF_P (X) && SYMBOL_REF_FLAGS (X) & SYMBOL_FLAG_PCREL)