Re: [PATCH][v3] GIMPLE store merging pass

[Jeff Law <law@redhat.com>]

On 09/07/2016 02:19 AM, Richard Biener wrote:
> On Tue, 6 Sep 2016, Jakub Jelinek wrote:
>
>> On Tue, Sep 06, 2016 at 04:59:23PM +0100, Kyrill Tkachov wrote:
>>> On 06/09/16 16:32, Jakub Jelinek wrote:
>>>> On Tue, Sep 06, 2016 at 04:14:47PM +0100, Kyrill Tkachov wrote:
>>>>> The v3 of this patch addresses feedback I received on the version posted at [1].
>>>>> The merged store buffer is now represented as a char array that we splat values onto with
>>>>> native_encode_expr and native_interpret_expr. This allows us to merge anything that native_encode_expr
>>>>> accepts, including floating point values and short vectors. So this version extends the functionality
>>>>> of the previous one in that it handles floating point values as well.
>>>>>
>>>>> The first phase of the algorithm that detects the contiguous stores is also slightly refactored according
>>>>> to feedback to read more fluently.
>>>>>
>>>>> Richi, I experimented with merging up to MOVE_MAX bytes rather than word size but I got worse results on aarch64.
>>>>> MOVE_MAX there is 16 (because it has load/store register pair instructions) but the 128-bit immediates that we ended
>>>>> synthesising were too complex. Perhaps the TImode immediate store RTL expansions could be improved, but for now
>>>>> I've left the maximum merge size to be BITS_PER_WORD.
>>>> At least from playing with this kind of things in the RTL PR22141 patch,
>>>> I remember storing 64-bit constants on x86_64 compared to storing 2 32-bit
>>>> constants usually isn't a win (not just for speed optimized blocks but also for
>>>> -Os).  For 64-bit store if the constant isn't signed 32-bit or unsigned
>>>> 32-bit you need movabsq into some temporary register which has like 3 times worse
>>>> latency than normal store if I remember well, and then store it.
>>>
>>> We could restrict the maximum width of the stores generated to 32 bits on x86_64.
>>> I think this would need another parameter or target macro for the target to set.
>>> Alternatively, is it a possibility for x86 to be a bit smarter in its DImode mov-immediate
>>> expansion? For example break up the 64-bit movabsq immediate into two SImode immediates?
>>
>> If you want a 64-bit store, you'd need to merge the two, and that would be
>> even more expensive.  It is a matter of say:
>> 	movl $0x12345678, (%rsp)
>> 	movl $0x09abcdef, 4(%rsp)
>> vs.
>> 	movabsq $0x09abcdef12345678, %rax
>> 	movq %rax, (%rsp)
>> vs.
>> 	movl $0x09abcdef, %eax
>> 	salq $32, %rax
>> 	orq $0x12345678, %rax
>> 	movq $rax, (%rsp)
>
> vs.
>
>         movq $LC0, (%rsp)
>
> ?
>
>> etc.  Guess it needs to be benchmarked on contemporary CPUs, I'm pretty sure
>> the last sequence is the worst one.
>
> I think the important part to notice is that it should be straight forward
> for a target / the expander to split a large store from an immediate
> into any of the above but very hard to do the opposite.  Thus from a
> GIMPLE side "canonicalizing" to large stores (that are eventually
> supported and well-aligned) seems best to me.
Agreed.


>
>>> I'm aware of that. The patch already has logic to avoid emitting unaligned accesses
>>> for SLOW_UNALIGNED_ACCESS targets. Beyond that the patch introduces the parameter
>>> PARAM_STORE_MERGING_ALLOW_UNALIGNED that can be used by the user or target to
>>> forbid generation of unaligned stores by the pass altogether. Beyond that I'm not sure
>>> how to behave more intelligently here. Any ideas?
>>
>> Dunno, the heuristics was the main problem with my patch.  Generally, I'd
>> say there is a difference between cold and hot blocks, in cold ones perhaps
>> unaligned stores are more appropriate (if supported at all and not way too
>> slow), while in hot ones less desirable.
>
> Note that I repeatedly argue that if we can canonicalize sth to "larger"
> then even if unaligned, the expander should be able to produce optimal
> code again (it might not do, of course).
And agreed.  Furthermore, it's in line with our guiding principles WRT 
separation of the tree/SSA optimizers from target dependencies.

So let's push those decisions into the expanders/backend/target and 
canonicalize to the larger stores.

jeff