On Fri, Jul 2, 2021 at 6:13 PM Jeff Law <j...@tachyum.com> wrote:
>
>
> This is a minor missed optimization we found with our internal port.
>
> Given this code:
>
> typedef struct {short a; short b;} T;
>
> extern void g1();
>
> void f(T s)
> {
> if (s.a < 0)
> g1();
> }
>
>
> "s" is passed in a register, but it's still a BLKmode object because the
> alignment of T is smaller than the alignment that an integer of the same
> size would have (16 bits vs 32 bits).
>
>
> Because "s" is BLKmode function.c is going to store it into a stack slot
> and we'll load it from the that slot for each reference. So on the v850
> (just to pick a port that likely has the same behavior we're seeing) we
> have this RTL from CSE2:
>
>
> (insn 2 4 3 2 (set (mem/c:SI (plus:SI (reg/f:SI 34 .fp)
> (const_int -4 [0xfffffffffffffffc])) [2 S4 A32])
> (reg:SI 6 r6)) "j.c":6:1 7 {*movsi_internal}
> (expr_list:REG_DEAD (reg:SI 6 r6)
> (nil)))
> (note 3 2 8 2 NOTE_INSN_FUNCTION_BEG)
> (insn 8 3 9 2 (set (reg:HI 44 [ s.a ])
> (mem/c:HI (plus:SI (reg/f:SI 34 .fp)
> (const_int -4 [0xfffffffffffffffc])) [1 s.a+0 S2 A32]))
> "j.c":7:5 3 {*movhi_internal}
> (nil))
> (insn 9 8 10 2 (parallel [
> (set (reg:SI 45)
> (ashift:SI (subreg:SI (reg:HI 44 [ s.a ]) 0)
> (const_int 16 [0x10])))
> (clobber (reg:CC 32 psw))
> ]) "j.c":7:5 94 {ashlsi3_clobber_flags}
> (expr_list:REG_DEAD (reg:HI 44 [ s.a ])
> (expr_list:REG_UNUSED (reg:CC 32 psw)
> (nil))))
> (insn 10 9 11 2 (parallel [
> (set (reg:SI 43)
> (ashiftrt:SI (reg:SI 45)
> (const_int 16 [0x10])))
> (clobber (reg:CC 32 psw))
> ]) "j.c":7:5 104 {ashrsi3_clobber_flags}
> (expr_list:REG_DEAD (reg:SI 45)
> (expr_list:REG_UNUSED (reg:CC 32 psw)
> (nil))))
>
>
> Insn 2 is the store into the stack. insn 8 is the load for s.a in the
> conditional. DSE1 replaces the MEM in insn 8 with (reg 6) since (reg 6)
> has the value we want. After that the store at insn 2 is dead. Sadly
> DSE never removes the store.
>
> The problem is RTL DSE considers a store with no MEM_EXPR as escaping,
> which keeps the MEM live. The lack of a MEM_EXPR is due to call to
> change_address to twiddle the mode on the MEM for the store at insn 2.
> It should be safe to copy the MEM_EXPR (which should always be a
> PARM_DECL) from the original memory to the memory returned by
> change_address. Doing so results in DSE1 removing the store at insn 2.
>
> It would be nice to remove the stack setup/teardown. I'm not offhand
> aware of mechanisms to remove the setup/teardown after we've already
> allocated a slot, even if the slot is no longer used.
>
> Bootstrapped and regression tested on x86, though I don't think that's a
> particularly useful test. So I also ran it through my tester across
> those pesky embedded targets without regressions as well.
>
> I didn't include a test simply because I didn't want to have an insane
> target selector. I guess if we really wanted a test we could look after
> DSE1 is done and verify there aren't any MEMs left at all. Willing to
> try that if the consensus is we want this tested.
>
> OK for the trunk?
I wonder why the code doesn't use adjust_address instead? That
handles most cases already and the code doesn't change the
address but just the mode (and access size)?
Richard.
> Jeff
>
>
