[PATCH] Relax check against commuting XOR and ASHIFTRT in combine.c

Alan Lawrence alan.lawrence@arm.com
Thu Sep 18 09:40:00 GMT 2014

Thanks for the reply - and the in-depth investigation. I agree that the
correctness of the compiler is critical rather than particular platforms such as
Ada / Alpha.

Moreover, I think we both agree that if result_mode==shift_mode, the
transformation is correct. "Just" putting that check in, achieves what I'm
trying for here, so I'd be happy to go with the attached patch and call it a
day. However, I'm a little concerned about the other cases - i.e. where
shift_mode is wider than result_mode.

If I understand correctly (and I'm not sure about that, let's see how far I
get), this means we'll perform the shift in (say) DImode, when we're only really
concerned about the lower (say) 32-bits (for an originally-SImode shift).
try_widen_shift_mode will in this case check that the result of the operation
*inside* the shift (in our case an XOR) has 33 sign bit copies (via
num_sign_bit_copies), i.e. that its *top* 32-bits are all equal to the original
SImode sign bit. <count> of these bits may be fed into the top of the desired
SImode result by the DImode shift. Right so far?

AFAICT, num_sign_bit_copies for an XOR, conservatively returns the minimum of
the num_sign_bit_copies of its two operands. I'm not sure whether this is
behaviour we should rely on in its callers, or for the sake of abstraction we
should treat num_sign_bit_copies as a black box (which does what it says on the,
erm, tin).

If the former, doesn't having num_sign_bit_copies >= the difference in size
between result_mode and shift_mode, of both operands to the XOR, guarantee
safety of the commutation (whether the constant is positive or negative)? We
could perform the shift (in the larger mode) on both of the XOR operands safely,
then XOR together their lower parts.

If, however, we want to play safe and ensure that we deal safely with any XOR
whose top (mode size difference + 1) bits were the same, then I think the
restriction that the XOR constant is positive is neither necessary nor
sufficient; rather (mirroring try_widen_shift_mode) I think we need that
num_sign_bit_copies of the constant in shift_mode, is more than the size
difference between result_mode and shift_mode.

Hmmm. I might try that patch at some point, I think it is the right check to
make. (Meta-comment: this would be *so*much* easier if we could write unit tests
more easily!) In the meantime I'd be happy to settle for the attached...

(tests are as they were posted
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01233.html .)


Jeff Law wrote:
> On 07/17/14 10:56, Alan Lawrence wrote:
>> Ok, the attached tests are passing on x86_64-none-linux-gnu,
>> aarch64-none-elf, arm-none-eabi, and a bunch of smaller platforms for
>> which I've only built a stage 1 compiler (i.e. as far as necessary to
>> assemble). That's with either change to simplify_shift_const_1.
>> As to the addition of "result_mode != shift_mode", or removing the whole
>> check against XOR - I've now tested the latter:
>> bootstrapped on x86_64-none-linux-gnu, check-gcc and check-ada;
>> bootstrapped on arm-none-linux-gnueabihf;
>> bootstrapped on aarch64-none-linux-gnu;
>> cross-tested check-gcc on aarch64-none-elf;
>> cross-tested on arm-none-eabi;
>> (and Uros has bootstrapped on alpha and done a suite of tests, as per
>> https://gcc.gnu.org/ml/gcc-testresults/2014-07/msg01236.html).
>>  From a perspective of paranoia, I'd lean towards adding "result_mode !=
>> shift_mode", but for neatness removing the whole check against XOR is
>> nicer. So I'd defer to the maintainers as to whether one might be
>> preferable to the other...(but my unproven suspicion is that the two are
>> equivalent, and no case where result_mode != shift_mode is possible!)
> So first, whether or not someone cares about Alpha-VMS isn't the issue 
> at hand.  It's whether or not the new code is correct or not. 
> Similarly the fact that the code generation paths are radically 
> different now when compared to 2004 and we can't currently trigger the 
> cases where the modes are different isn't the issue, again, it's whether 
> or not your code is correct or not.
> I think the key is to look at try_widen_shift_mode and realize that it 
> can return a mode larger than the original mode of the operations. 
> However, it only does so when it presented with a case where it knows 
> the sign bit being shifted in from the left will be the same as the sign 
> bit in the original mode.
> In the case of an XOR when the sign bit set in shift_mode, that's not 
> going to be the case.  We would violate the assumption made when we 
> decided to widen the shift to shift_mode.
> So your relaxation is safe when shift_mode == result_mode, but unsafe 
> otherwise -- even though we don't currently have a testcase for the 
> shift_mode != result_mode case, we don't want to break that.
> So your updated patch is correct.  However, I would ask that you make 
> one additional change.  Namely the comment before the two fragments of 
> code you changed needs updating.  Something like
> "... and the constant has its sign bit set in shift_mode and shift_mode
>   is different than result_mode".
> The 2nd comment should have similar clarifying comments.
> You should also include your testcase for a cursory review.
> So with the inclusion of the testcase and updated comments, we should be 
> good to go.  However, please post the final patch for that cursory 
> review of the testcase.
> jeff
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ashiftrt_xor.patch
Type: text/x-patch
Size: 6810 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20140918/d740fca1/attachment.bin>

More information about the Gcc-patches mailing list