Hi Bernd,
On 04/07/16 19:02, Bernd Schmidt wrote:
On 07/01/2016 11:18 AM, Kyrill Tkachov wrote:
In this arm wrong-code PR the struct assignment goes wrong when
expanding constructor elements to a register destination
when the constructor elements are signed bitfields less than a word wide.
In this testcase we're intialising a struct with a 16-bit signed
bitfield to -1 followed by a 1-bit bitfield to 0.
Before it starts storing the elements it zeroes out the register.
The code in store_constructor extends the first field to word size
because it appears at the beginning of a word.
It sign-extends the -1 to word size. However, when it later tries to
store the 0 to bitposition 16 it has some logic
to avoid redundant zeroing since the destination was originally cleared,
so it doesn't emit the zero store.
But the previous sign-extended -1 took up the whole word, so the
position of the second bitfield contains a set bit.
This patch fixes the problem by zeroing out the bits of the widened
field that did not appear in the original value,
so that we can safely avoid storing the second zero in the constructor.
[...]
Bootstrapped and tested on arm, aarch64, x86_64 though the codepath is
gated on WORD_REGISTER_OPERATIONS I didn't
expect any effect on aarch64 and x86_64 anyway.
So - that code path starts with this comment:
/* If this initializes a field that is smaller than a
word, at the start of a word, try to widen it to a full
word. This special case allows us to output C++ member
function initializations in a form that the optimizers
can understand. */
Doesn't your patch completely defeat the purpose of this? Would you get
better/identical code by just deleting this block? It seems unfortunate to have
two different code generation approaches like this.
It would be interesting to know the effects of your patch, and the effects of removing this code entirely, on generated code. Try to find the motivating C++ member function example perhaps? Maybe another possibility is to ensure this
doesn't happen if the value would be interpreted as signed.
Doing some archaeology shows this code was added back in 1998 with no testcase
(r22567) so I'd have to do more digging.
My interpretation of that comment was that for WORD_REGISTER_OPERATIONS targets
it's more beneficial to have word-size
operations, so the expansion code tries to emit as many of the operations in
word_mode as it safely can.
With my patch it still emits a word_mode operation, it's just that the
immediate that is moved in word_mode has it's
top bits zeroed out instead of sign-extended.
I'll build SPEC2006 on arm (a WORD_REGISTER_OPERATIONS target) and inspect the
assembly to see if I see any interesting
effects, but I suspect there won't be much change.
Thanks for looking at this,
Kyrill
Bernd
