[ARM] Fix PR50313 and handle PIC addresses properly.

Hi ,

PR50313 is a case where having the 2 patterns "pic_load_addr_*" and
"pic_add_dot_plus_eight/four" from expand time becomes a problem as
discussed by Jakub in his comment in the audit trail for PR48308.
There is a separate problem in combine as explained by my comment in
the audit trail for PR48308 and my RFC patch for the same sometime
last week.

What I've done in this case is to take the existing patterns merged
them into a UNIFIED form which means that we can hoist the entire
computation out with any of the loop optimizers rather than piece-meal
hoping to have each one being hoisted correctly. I've had to adjust
the cost of this to be equivalent to that of a memory load ( it's in
fact the cost of a memory load + the cost of an add instruction but
it's good enough for the moment). It's reasonably close enough for
arm_size_rtx_costs, and thumb1_rtx_costs pushes this high enough that
it will warrant a hoist anyway. I've also been conservative in
adjusting cannot_copy_insn_p to disallow copying such insns to prevent
any cases where you might have duplicate labels being generated.

Once this happens the original bug report in PR50313 appears to be
fixed and I've had this survive a bootstrap and regression test on an
A9 board and this has successfully cross-built a version of eglibc
with an ARM v7-a configuration (which is currently undergoing
testing).

There are a few ways by which we can get better code - one is to
replace the load from the constant pool with a movw / movt pair of the
actual differences in which case we'd be able to get performant code
in libraries for this case, the other which as RichardE suggested in a
conversation was to allow the splitter to actually generate the
internal labels in the pic_load_addr and pic_add_dot_plus_eight which
could then mean that the label number is really not reserved until
this complex pattern has been split. I don't think these enhancements
are worth doing so late in stage4 and I do want to backport this patch
as it stands into the 4.6 branch.

OK (and to backport to 4.6 branch ?) if no regressions ?

cheers
Ramana


2012-01-18  Ramana Radhakrishnan  <ramana.radhakrishnan@linaro.org>

	PR target/50313
	* config/arm/arm.c (arm_load_pic_register): Use
	gen_pic_load_addr_unified. Delete calls to gen_pic_load_addr_32bit
	, gen_pic_add_dot_plus_eight and gen_pic_add_dot_plus_four.
	(arm_pic_static_addr): Likewise.
	(arm_rtx_costs_1): Adjust cost for UNSPEC_PIC_UNIFIED.
	(arm_note_pic_base): Handle UNSPEC_PIC_UNIFIED.
	* config/arm/arm.md (UNSPEC_PIC_UNIFIED): Define.
	(pic_load_addr_unified): New.

On Wed, Jan 18, 2012 at 03:03:47PM +0000, Ramana Radhakrishnan wrote:
> +;; operand1 is the memory address to go into pic_load_addr_32bit.
> +;; operand2 is the PIC label to be emitted from pic_add_dot_plus_*.
> +;; We do this to allow hoisting of the entire c
> +(define_insn_and_split "pic_load_addr_unified"
> +  [(set (match_operand:SI 0 "s_register_operand" "=r,r,l")
> +	(unspec:SI [(match_operand:SI 1 "" "mX,mX,mX")
> +		    (match_operand:SI 2 "" "")]
> +		    UNSPEC_PIC_UNIFIED))]
> + "flag_pic"
> + "#"
> + "flag_pic"

What's the reason to duplicate the condition here?  If flag_pic isn't
non-zero, then the insn wouldn't match, therefore wouldn't be split...

Also, while this fixes the testcase, at least if you split it before reload
e.g. first scheduling pass might move away the two insns far away from each
other, register allocation might decide to spill the pseudo set by the first
insn to the stack and the same problem could reappear.
That doesn't mean your patch isn't an incremental improvement.

> + [(set (match_dup 3) (unspec:SI [(match_dup 1)] UNSPEC_PIC_SYM))
> +  (set (match_dup 0) (unspec:SI [(match_dup 3) (match_dup 4)
> +       		     		 (match_dup 2)] UNSPEC_PIC_BASE))]
> + "operands[3] = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
> +  operands[4] = TARGET_THUMB ? GEN_INT (4) : GEN_INT (8);"
> + [(set_attr "type" "load1,load1,load1")
> +  (set_attr "pool_range" "4096,4096,1024")
> +  (set_attr "neg_pool_range" "0,4084,0")
> +  (set_attr "arch"  "a,t2,t1")
> +  (set_attr "length" "8,6,4")]
> +)
> +

	Jakub

On 18 January 2012 15:26, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jan 18, 2012 at 03:03:47PM +0000, Ramana Radhakrishnan wrote:
>> +;; operand1 is the memory address to go into pic_load_addr_32bit.
>> +;; operand2 is the PIC label to be emitted from pic_add_dot_plus_*.
>> +;; We do this to allow hoisting of the entire c
>> +(define_insn_and_split "pic_load_addr_unified"
>> +  [(set (match_operand:SI 0 "s_register_operand" "=r,r,l")
>> +     (unspec:SI [(match_operand:SI 1 "" "mX,mX,mX")
>> +                 (match_operand:SI 2 "" "")]
>> +                 UNSPEC_PIC_UNIFIED))]
>> + "flag_pic"
>> + "#"
>> + "flag_pic"
>
> What's the reason to duplicate the condition here?  If flag_pic isn't
> non-zero, then the insn wouldn't match, therefore wouldn't be split...

True -

>
> Also, while this fixes the testcase, at least if you split it before reload
> e.g. first scheduling pass might move away the two insns far away from each
> other, register allocation might decide to spill the pseudo set by the first
> insn to the stack and the same problem could reappear.

Since this was always being scheduled as a standard load I didn't have
expected this to be pulled too far apart from the use - we only
schedule loads as though these were off the cache anyway. I will
change to get this split after reload to make this as robust as it
possibly can be .

I will note that the tls patterns also would need a similar overhaul
but I don't want to do that till stage1 reopens again.

There is another problem with this patch in that I've got the
neg_pool_range for ARM and Thumb2 back to front - will change that and
then commit.


regards,
Ramana