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.
Bernd
