Re: Bad CC0 optimizer in VAX backend (was Re: Patches to fix GCC’s C++ exception handling on NetBSD/VAX)

[Jeff Law <law%redhat.com@localhost>]

On 03/28/2016 04:29 PM, Jake Hamby wrote:
> I have some bad news and some good news. The bad news is that there
> has been a nasty optimizer bug lurking in the VAX backend for GCC for
> many years, which has to do with over-optimistic removal of necessary
> tst/cmp instructions under certain circumstances. This manifests at
> -O or higher and the symptoms are strange behavior like stack
> overflows because of branches going the wrong way.
So let's get that addressed.  Matt would be the best person to review 
this change as he's the Vax maintainer.  But if it's simple and 
straightforward others can handle.

It will make Matt's job easier if you can create a self-contained 
testcases which show these problems.  Ideally it'll exit (0) when the 
test passes and abort() when the test fails.  Matt (or someone else) can 
use that to help verify exactly what is happening *and* the test can be 
added to the regression testsuite.

There are hints for testcase reduction on the gcc website.

> The good news is that my suspicions about the CC0 handler appear to
> be correct, and better yet, I'm currently testing a patch that isn't
> too big and I believe will fix this bug. The bad behavior is in
> vax_notice_update_cc (), which is supposed to call CC_STATUS_INIT if
> the CC flags have been reset, or set cc_status.value1 and possibly
> cc_status.value2 with the destination of the current (set...)
> instruction, whose signed comparison to 0 will be stored in the N and
> Z flags as a side effect (most VAX instructions do this).
Right.  Pretty standard stuff for a cc0 target.


> The first bug is that most, but not all of the define_insn patterns
> in vax.md actually do this. The paths that emit movz* instructions
> will not set N and Z, and there are some code paths that can't be
> trusted because they handle a 64-bit integer in two 32-bit pieces,
> for example, so N and Z won't reflect the desired result (comparison
> of operand 0 to 0).
Understood.  So it sounds like there's two bugs, which implies we'd 
really like two independent tests.

One is the movz instructions don't set the cc0 codes, but 
notice_update_cc isn't aware of that or doesn't handle it properly.

Second is the handling of notice_update_cc for double-word instructions. 
 Many ports use CC_STATUS_INIT for those as tracking the status bits 
can be painful.

> The current version of vax_notice_update_cc has a specific check for
> this: it won't set the cc_status cache if GET_CODE (SET_DEST (exp))
> != ZERO_EXTRACT. The problem is that this is checking the RTL
> expression for (zero_extract...) and not whether what was actually
> emitted is a movz* or not. That's why all of the define_insn()'s have
> to be annotated with their actual behavior vis-a-vis compare to 0,
> and then the change to vax.c to read the CC attribute makes it really
> much easier to get the correct behavior.
Right.  That's similar to how other cc0 ports work -- they have 
attributes which clearly indicate the cc0 status for each alternative. 
Then you just need to add the attribute to each insn and check it in 
notice_update_cc.  See the v850 port as a fairly simple example of how 
this can be handled.


> I need to do a lot of testing today to see if this really does help
> fix GCC's VAX backend, but I'm hopeful that this is at least a real
> bug that needs to be fixed, and that I have a fix for it that should
> work. The other good news is that you only need three cc enums:
> "none" (doesn't alter Z or N flags), "compare" (Z and N reflect
> comparison of operand 0 to literal 0), and "clobber" (call
> CC_STATUS_INIT to reset cc_status cache).
It's not uncommon to need an attribute that says the cc0 bits aren't 
modified, but operand0 is modified.  In fact, that may be what you want 
for the movz instructions on the vax from what I've read above.

As far as the patch itself, I'd drop all the grubbing around in RTL, 
except for the case where the cc0 bits are set from a comparison. 
Essentially you let the insn's attributes entirely describe the cc0 
status.   Again, see v850.c::notice_update_cc and the attributes in v850.md.

Jeff