Re: [PATCH,4.4/4.5/4.6?] check for constant base addresses in is_gimple_invariant_address

[Richard Guenther <richard dot guenther at gmail dot com>]

On Sun, Sep 5, 2010 at 4:12 AM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> The functions in the following testcase (PPC-specific, but
> generalizable) do not compile to the same code:
>
> #define ASMSTR "stw%U0%X0 %1,%0"
>
> struct Foo
> {
> ?unsigned pad[10];
> ?unsigned cs0_bnds;
> };
>
> static inline
> void Out (unsigned *addr)
> {
> ?__asm__ __volatile__(ASMSTR : "=m" (*addr) : "r" (0x1f));
> }
>
> void AsmFunction (void)
> {
> ?struct Foo *ddr = (struct Foo *)0xffe02000;
>
> ?Out (&ddr->cs0_bnds);
> ?Out (&ddr->pad[0]);
> }
>
> void AsmDirect (void)
> {
> ?struct Foo *ddr = (struct Foo *)0xffe02000;
>
> ?__asm__ __volatile__(ASMSTR : "=m" (ddr->cs0_bnds) : "r" (0x1f));
> ?__asm__ __volatile__(ASMSTR : "=m" (ddr->pad[0]) : "r" (0x1f));
> }
>
> The results are (cleaned up to remove #APP bits):
>
> AsmFunction:
> ? ? ? ?lis 9,0xffe0
> ? ? ? ?li 0,31
> ? ? ? ?ori 9,9,8232
> ? ? ? ?stw 0,0(9)
> ? ? ? ?lis 9,0xffe0
> ? ? ? ?ori 9,9,8192
> ? ? ? ?stw 0,0(9)
> ? ? ? ?blr
>
> AsmDirect:
> ? ? ? ?lis 9,0xffe0
> ? ? ? ?li 0,31
> ? ? ? ?ori 9,9,8192
> ? ? ? ?stw 0,40(9)
> ? ? ? ?li 0,28
> ? ? ? ?stw 0,0(9)
> ? ? ? ?blr
>
> Notice that in AsmDirect, we've CSE'd the address of `ddr', and we
> haven't done so in AsmFunction. ?(We failed to move the lowpart of the
> address into the stores, but AFAICT that is a problem with the PPC
> backend and is not relevant here. ?Compiling the example above for MIPS
> demonstrates the same problem without the distraction of bad code.)
>
> We used to do this CSE'ing in 4.3. ?When faced with this code:
>
> ?D.2017_2 = &4292878336B->cs0_bnds;
> ?__asm__ __volatile__("stw%U0%X0 %1,%0" : "=m" *D.2017_2 : "r" 31);
>
> in ccp, we'd propagate D.2017_2 into its use and wind up with identical
> trees between the two functions heading into expand. ?In 4.4 and 4.5,
> however, we don't, so we've never given an opportunity to CSE the
> address of `ddr'.
>
> This affects applications that are size-conscious; if you're doing
> several calls to 'Out' in the same function, your code size can increase
> quite a bit in 4.4 and 4.5.
>
> So far as I can tell, this is because is_gimple_invariant_address fails
> to consider that we might have an INTEGER_CST base address. ?The patch
> below fixes the testcase; with it applied, AsmFunction and AsmDirect
> compile to the same code. ?What I'm unsure of is:
>
> - Whether I should also check for decl_address_invariant_p, perhaps by:
>
> ?if (TREE_CODE (op) == INDIRECT_REF)
> ? ?op = TREE_OPERAND (op, 0);
>
> ?return CONSTANT_CLASS_P (op) || decl_address_invariant_p (op);
>
> - Whether the same treatment should be applied to
> ?is_gimple_ip_invariant_address.
>
> My most recent powerpc compiler is somewhat old, but it is post-MEM_REF
> merge and it also does not do the desired propagation. ?It looks to me
> like the relevant case in handled in is_gimple_invariant_address, but
> something is not working; the result &4292878336B->FOO is marked
> VARYING.
>
> Anyway, the below patch is against 4.5 and I'm asking for approval to
> 4.4 and 4.5 and possibly mainline (with modifications, obviously) once I
> verify with an up-to-date compiler. ?Answers to the questions above
> would be helpful.
>
> -Nathan
>
> ? ? ? ?* gimple.c (is_gimple_invariant_address): Check for a constant
> ? ? ? ?operand of an INDIRECT_REF.
>
> Index: gcc/ChangeLog
> ===================================================================
> Index: gcc/gimple.c
> ===================================================================
> --- gcc/gimple.c ? ? ? ?(revision 163868)
> +++ gcc/gimple.c ? ? ? ?(working copy)
> @@ -2591,7 +2591,13 @@ is_gimple_invariant_address (const_tree
>
> ? op = strip_invariant_refs (TREE_OPERAND (t, 0));
>
> - ?return op && (CONSTANT_CLASS_P (op) || decl_address_invariant_p (op));
> + ?if (!op)
> + ? ?return false;
> +
> + ?if (TREE_CODE (op) == INDIRECT_REF)
> + ? ?return CONSTANT_CLASS_P (TREE_OPERAND (op, 0));
> +
> + ?return (CONSTANT_CLASS_P (op) || decl_address_invariant_p (op));
> ?}

This doesn't make sense.  If we are asking for &CST->x then we should have
folded it to a plain CST.

So - you have to look elsewhere for why that does not happen.

(and of course such a change wouldn't be appropriate for the release branches,
you basically change the GIMPLE IL)

Richard.

> ?/* Return true if T is a gimple invariant address at IPA level
>