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
