Side effects on memory access

Side effects on memory access

[Michael Hope]

Hi there.  I'm looking at porting GCC to a new architecture which has
a quite small instruction set and I'm afraid I can't figure out how to
represent unintended side effects on instructions.

My current problem is accessing memory.  Reading an aligned 32 bit
word is simple using LOADACC, (X).  Half words and bytes are harder as
the only instruction available is a load byte with post increment
'LOADACC, (X+)'.

How can I tell GCC that loading a byte also increases the pointer
register?  My first version reserved one of the pointer registers and
threw away the modified value but this is inefficient.  I suspect that
some type of clobber or define_expand is required but I can't figure
it out.

Thanks for any help,

-- Michael

Re: Side effects on memory access

[Ian Lance Taylor]

Michael Hope <michaelh@juju.net.nz> writes:

> Hi there.  I'm looking at porting GCC to a new architecture which has
> a quite small instruction set and I'm afraid I can't figure out how to
> represent unintended side effects on instructions.
>
> My current problem is accessing memory.  Reading an aligned 32 bit
> word is simple using LOADACC, (X).  Half words and bytes are harder as
> the only instruction available is a load byte with post increment
> 'LOADACC, (X+)'.

Wow.

> How can I tell GCC that loading a byte also increases the pointer
> register?  My first version reserved one of the pointer registers and
> threw away the modified value but this is inefficient.  I suspect that
> some type of clobber or define_expand is required but I can't figure
> it out.

Well, you can use a define_expand to generate the move in the first
place.  If can_create_pseudo_p() returns true, then you can call
copy_to_reg (addr) to get the address into a register, and you can
generate the post increment.

(define_expand "movhi"
  ...
  if (can_create_pseudo_p () && MEM_P (operands[1]))
    {
      rtx reg = copy_to_reg (XEXP (operands[1], 0));
      emit_insn (gen_movhi_insn (operands[0], reg));
      DONE;
    }
  ...
)

(define_insn "movhi_insn"
  [(set (match_operand:HI 0 ...)
        (mem:HI (post_inc:P (match_operand:P 1 "register_operand" ...))))]
  ...
)

The difficulties are going to come in reload.  Reload will want to load
and store 16-bit values in order to spill registers.  You will need a
scratch register to dothis, and that means that you need to implement
TARGET_SECONDARY_RELOAD.  This is complicated:read the docs carefully
and look at the existing examples.

Ian

Re: Side effects on memory access

[Michael Hope]

Thanks for the response Ian.  Doing the define_expand inserts the post
increment but GCC doesn't seem to notice the change in X.

I added this code:

----
(define_expand "movqi"
  [(set (match_operand:QI 0 "nonimmediate_operand")
	(match_operand:QI 1 "general_operand" ""))]
  ""
  {
    if (can_create_pseudo_p () && MEM_P (operands[1]))
      {
	rtx reg = copy_to_reg (XEXP (operands[1], 0));
	emit_insn (gen_movqi_mem (operands[0], reg));
	DONE;
      }
  }
)

; PENDING: The SI here is actually a P
(define_insn "movqi_mem"
  [(set (match_operand:QI 0 "register_operand" "=d")
	(mem:QI (post_inc:SI (match_operand:SI 1 "register_operand" "a"))))]
  ""
  "LOADACC, (%1+)\;STOREACC, %0"
)
----
The 'd' constraint is for data registers and the 'a' for address
registers, which is only the X register due to cache coherency
reasons.

When compiling this test case:
----
uint store5(volatile char* p)
{
  return *p + *p;
}
----
I get the following move2.i.139r.subreg:
---
(insn 3 5 4 2 move2.c:56 (set (reg/v/f:SI 30 [ p ])
        (reg:SI 5 R10 [ p ])) 6 {movsi} (nil))

(note 4 3 7 2 NOTE_INSN_FUNCTION_BEG)

(insn 7 4 8 2 move2.c:57 (set (reg:QI 31)
        (mem:QI (post_inc:SI (reg/v/f:SI 30 [ p ])) [0 S1 A8])) 0
{movqi_mem} (nil))

(insn 8 7 9 2 move2.c:57 (set (reg:SI 27 [ D.1191 ])
        (zero_extend:SI (reg:QI 31))) 24 {zero_extendqisi2} (nil))

(insn 9 8 10 2 move2.c:57 (set (reg:QI 32)
        (mem:QI (post_inc:SI (reg/v/f:SI 30 [ p ])) [0 S1 A8])) 0
{movqi_mem} (nil))

(insn 10 9 11 2 move2.c:57 (set (reg:SI 26 [ D.1193 ])
        (zero_extend:SI (reg:QI 32))) 24 {zero_extendqisi2} (nil))

(insn 11 10 12 2 move2.c:57 (set (reg:SI 33)
        (plus:SI (reg:SI 26 [ D.1193 ])
            (reg:SI 27 [ D.1191 ]))) 9 {addsi3} (nil))

(insn 12 11 16 2 move2.c:57 (set (reg:SI 28 [ <result> ])
        (reg:SI 33)) 6 {movsi} (nil))

(insn 16 12 22 2 move2.c:58 (set (reg/i:SI 5 R10)
        (reg:SI 28 [ <result> ])) 6 {movsi} (nil))

(insn 22 16 0 2 move2.c:58 (use (reg/i:SI 5 R10)) -1 (nil))
---
Instruction 3 copies incoming argument in R10 is copied into pseudo
30.  Pseudo 30 is then used at instruction 7 then instruction 9
without either being reloaded or corrected for the post increment.

-- Michael

2009/4/22 Ian Lance Taylor <iant@google.com>:
> Michael Hope <michaelh@juju.net.nz> writes:
>
>> Hi there.  I'm looking at porting GCC to a new architecture which has
>> a quite small instruction set and I'm afraid I can't figure out how to
>> represent unintended side effects on instructions.
>>
>> My current problem is accessing memory.  Reading an aligned 32 bit
>> word is simple using LOADACC, (X).  Half words and bytes are harder as
>> the only instruction available is a load byte with post increment
>> 'LOADACC, (X+)'.
>
> Wow.
>
>> How can I tell GCC that loading a byte also increases the pointer
>> register?  My first version reserved one of the pointer registers and
>> threw away the modified value but this is inefficient.  I suspect that
>> some type of clobber or define_expand is required but I can't figure
>> it out.
>
> Well, you can use a define_expand to generate the move in the first
> place.  If can_create_pseudo_p() returns true, then you can call
> copy_to_reg (addr) to get the address into a register, and you can
> generate the post increment.
>
> (define_expand "movhi"
>  ...
>  if (can_create_pseudo_p () && MEM_P (operands[1]))
>    {
>      rtx reg = copy_to_reg (XEXP (operands[1], 0));
>      emit_insn (gen_movhi_insn (operands[0], reg));
>      DONE;
>    }
>  ...
> )
>
> (define_insn "movhi_insn"
>  [(set (match_operand:HI 0 ...)
>        (mem:HI (post_inc:P (match_operand:P 1 "register_operand" ...))))]
>  ...
> )
>
> The difficulties are going to come in reload.  Reload will want to load
> and store 16-bit values in order to spill registers.  You will need a
> scratch register to dothis, and that means that you need to implement
> TARGET_SECONDARY_RELOAD.  This is complicated:read the docs carefully
> and look at the existing examples.
>
> Ian
>

Re: Side effects on memory access

[Ian Lance Taylor]

Michael Hope <michaelh@juju.net.nz> writes:

> (define_expand "movqi"
>   [(set (match_operand:QI 0 "nonimmediate_operand")
> 	(match_operand:QI 1 "general_operand" ""))]
>   ""
>   {
>     if (can_create_pseudo_p () && MEM_P (operands[1]))
>       {
> 	rtx reg = copy_to_reg (XEXP (operands[1], 0));
> 	emit_insn (gen_movqi_mem (operands[0], reg));
> 	DONE;
>       }
>   }
> )

It's hideous, but you're going to need to do

    insn = emit_insn (gen_movqi_mem (operands[0], reg));
    add_reg_note (insn, REG_INC, reg);

add_reg_note is pretty new; if you don't have it in your sources, it
amounts to

    REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_INC, reg, REG_NOTES (insn));

Ian

Re: Side effects on memory access

[Michael Hope]

No luck on that.  I've re-baselined off GCC 4.4.0 to get the
add_reg_note function() but the register is still re-used wihtout
being reloaded.

The test case is:
--
uint32_t load_q(volatile uint8_t* p)
{
  return *p + *p;
}
--
The appropriate section of the md file is:
---
(define_expand "movqi"
  [(set (match_operand:QI 0 "nonimmediate_operand")
	(match_operand:QI 1 "general_operand" ""))]
  ""
  {
    if (can_create_pseudo_p () && MEM_P (operands[1]))
      {
	rtx reg = copy_to_reg (XEXP (operands[1], 0));
        rtx insn = emit_insn (gen_movqi_mem (operands[0], reg));
        add_reg_note (insn, REG_INC, reg);
	DONE;
      }
  }
)

(define_insn "movqi_mem"
  [(set (match_operand:QI 0 "register_operand" "=d")
	(mem:QI (post_inc:SI (match_operand:SI 1 "register_operand" "a"))))]
  ""
  "LOADACC, (%1+)\;STOREACC, %0"
)
---
My last RTL dump was wrong due to it hitting a zero extend from memory
optimisation.  However, this time test.i.136r.subreg1 contains:
---
(insn 3 5 4 2 loads.c:4 (set (reg/v/f:SI 30 [ p ])
        (reg:SI 5 R10 [ p ])) 6 {movsi} (nil))

(note 4 3 7 2 NOTE_INSN_FUNCTION_BEG)

(insn 7 4 8 2 loads.c:5 (set (reg:SI 32)
        (reg/v/f:SI 30 [ p ])) 6 {movsi} (nil))

(insn 8 7 9 2 loads.c:5 (set (reg:QI 31)
        (mem:QI (post_inc:SI (reg:SI 32)) [0 S1 A8])) 0 {movqi_mem}
(expr_list:REG_INC (reg:SI 32)
        (nil)))

(insn 9 8 10 2 loads.c:5 (set (reg:SI 27 [ D.1215 ])
        (zero_extend:SI (reg:QI 31))) 24 {zero_extendqisi2} (nil))

(insn 10 9 11 2 loads.c:5 (set (reg:SI 34)
        (reg/v/f:SI 30 [ p ])) 6 {movsi} (nil))

(insn 11 10 12 2 loads.c:5 (set (reg:QI 33)
        (mem:QI (post_inc:SI (reg:SI 34)) [0 S1 A8])) 0 {movqi_mem}
(expr_list:REG_INC (reg:SI 34)
        (nil)))

(insn 12 11 13 2 loads.c:5 (set (reg:SI 26 [ D.1217 ])
        (zero_extend:SI (reg:QI 33))) 24 {zero_extendqisi2} (nil))

(insn 13 12 14 2 loads.c:5 (set (reg:SI 35)
        (plus:SI (reg:SI 26 [ D.1217 ])
            (reg:SI 27 [ D.1215 ]))) 9 {addsi3} (nil))
---
This is correct so far, but the next step in test.i.138r.cse1 contains:
---
(insn 3 5 4 2 loads.c:4 (set (reg/v/f:SI 30 [ p ])
        (reg:SI 5 R10 [ p ])) 6 {movsi} (nil))

(note 4 3 7 2 NOTE_INSN_FUNCTION_BEG)

(insn 7 4 8 2 loads.c:5 (set (reg/f:SI 32 [ p ])
        (reg/v/f:SI 30 [ p ])) 6 {movsi} (nil))

(insn 8 7 9 2 loads.c:5 (set (reg:QI 31)
        (mem:QI (post_inc:SI (reg/v/f:SI 30 [ p ])) [0 S1 A8])) 0
{movqi_mem} (expr_list:REG_INC (reg/f:SI 32 [ p ])
        (nil)))

(insn 9 8 10 2 loads.c:5 (set (reg:SI 27 [ D.1215 ])
        (zero_extend:SI (reg:QI 31))) 24 {zero_extendqisi2} (nil))

(insn 10 9 11 2 loads.c:5 (set (reg/f:SI 34 [ p ])
        (reg/v/f:SI 30 [ p ])) 6 {movsi} (nil))

(insn 11 10 12 2 loads.c:5 (set (reg:QI 33)
        (mem:QI (post_inc:SI (reg/v/f:SI 30 [ p ])) [0 S1 A8])) 0
{movqi_mem} (expr_list:REG_INC (reg/f:SI 34 [ p ])
        (nil)))

(insn 12 11 13 2 loads.c:5 (set (reg:SI 26 [ D.1217 ])
        (zero_extend:SI (reg:QI 33))) 24 {zero_extendqisi2} (nil))

(insn 13 12 14 2 loads.c:5 (set (reg:SI 35)
        (plus:SI (reg:SI 26 [ D.1217 ])
            (reg:SI 27 [ D.1215 ]))) 9 {addsi3} (nil))
---
At this level pseudo register 30 is being used in each load without
being invalidated or re-loaded.

-- Michael

Re: Side effects on memory access

[Ian Lance Taylor]

Michael Hope <michaelh@juju.net.nz> writes:

> My last RTL dump was wrong due to it hitting a zero extend from memory
> optimisation.  However, this time test.i.136r.subreg1 contains:

> (insn 7 4 8 2 loads.c:5 (set (reg:SI 32)
>         (reg/v/f:SI 30 [ p ])) 6 {movsi} (nil))
>
> (insn 8 7 9 2 loads.c:5 (set (reg:QI 31)
>         (mem:QI (post_inc:SI (reg:SI 32)) [0 S1 A8])) 0 {movqi_mem}
> (expr_list:REG_INC (reg:SI 32)
>         (nil)))

> This is correct so far, but the next step in test.i.138r.cse1 contains:

> (insn 7 4 8 2 loads.c:5 (set (reg/f:SI 32 [ p ])
>         (reg/v/f:SI 30 [ p ])) 6 {movsi} (nil))
>
> (insn 8 7 9 2 loads.c:5 (set (reg:QI 31)
>         (mem:QI (post_inc:SI (reg/v/f:SI 30 [ p ])) [0 S1 A8])) 0
> {movqi_mem} (expr_list:REG_INC (reg/f:SI 32 [ p ])
>         (nil)))

This substitution is clearly invalid.  So there is a bug in CSE.  Most
likely this bug has not been noticed before because POST_INC and friends
are normally inserted by the inc_dec pass which runs after CSE.

It may be that all that is needed is to change the cse_insn function to
look for REG_INC notes.

Ian

Re: Side effects on memory access

[Michael Hope]

Thanks.  I'm going to work around it for now by post correcting X -
it's a hack but I'm in the early stages of the port so I can get back
to it later.

-- Michael

2009/4/28 Ian Lance Taylor <iant@google.com>:
> Michael Hope <michaelh@juju.net.nz> writes:
>
>> My last RTL dump was wrong due to it hitting a zero extend from memory
>> optimisation.  However, this time test.i.136r.subreg1 contains:
>
>> (insn 7 4 8 2 loads.c:5 (set (reg:SI 32)
>>         (reg/v/f:SI 30 [ p ])) 6 {movsi} (nil))
>>
>> (insn 8 7 9 2 loads.c:5 (set (reg:QI 31)
>>         (mem:QI (post_inc:SI (reg:SI 32)) [0 S1 A8])) 0 {movqi_mem}
>> (expr_list:REG_INC (reg:SI 32)
>>         (nil)))
>
>> This is correct so far, but the next step in test.i.138r.cse1 contains:
>
>> (insn 7 4 8 2 loads.c:5 (set (reg/f:SI 32 [ p ])
>>         (reg/v/f:SI 30 [ p ])) 6 {movsi} (nil))
>>
>> (insn 8 7 9 2 loads.c:5 (set (reg:QI 31)
>>         (mem:QI (post_inc:SI (reg/v/f:SI 30 [ p ])) [0 S1 A8])) 0
>> {movqi_mem} (expr_list:REG_INC (reg/f:SI 32 [ p ])
>>         (nil)))
>
> This substitution is clearly invalid.  So there is a bug in CSE.  Most
> likely this bug has not been noticed before because POST_INC and friends
> are normally inserted by the inc_dec pass which runs after CSE.
>
> It may be that all that is needed is to change the cse_insn function to
> look for REG_INC notes.
>
> Ian
>