On 18 January 2012 15:26, Jakub Jelinek <ja...@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
