On Mon, 22 Apr 2024 15:07:59 PDT (-0700), juzhe.zh...@rivai.ai wrote:
Apologize that we didn't post our (me, kito and Li Pan) disscussions.
Some amount of off-list discussion is inevitable, but as far as I can
tell here we ended up with some code committed that wasn't even posted
to the lists and didn't even build. I don't know exactly where the bar
for public discussions is, but it's got to be somewhere higher than
that.
This is the story:
We found that my previous patches which support highpart register overlap with
register filter for instructions like (vwadd.wv)
cause ICE reported by:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114714
and this is obviously a regression (No ICE on GCC 13.2, but ICE on GCC 14)
We have tried several fixes to work around this ICE, however, we failed.
And also I found my previous patches are quite wrong which is not the perfect
solution to support register group overlap
for vwadd.wv.
So, finally we decide to revert those patches.
Sure, reverting something that has a bug is reasonable. The problem is
how this happened: this code clearly did not get tested, as it doesn't
even build and re-introduces a bunch of ICEs. We're very late in stage
4 and this is the second time the entire port has been broken in as many
weeks. That's a really bad time to be breaking things.
I think we're really set the wrong precedent on what the bar is for
review here. We had a lot of breakages early on in the 14 development
cycle and never really dug into that, I was hoping that getting CI set
up would be a strong enough hint to stop the breakages. Clearly that
didn't work, though, so:
Please stop breaking the port.
It's exceedingly rare that any patch needs to be committed minutes after
it's posted. These port-wide breakages are really the only thing where
it could be agreed that's the way to go, but as we can see here rushing
is as likely to dig a bigger hole as it is to fix things. I get the
testsuite can be kind of hard to run, but if you don't want to run it
locally you can just wait for the CI to do it for you. That's not
really asking for very much.
Kito knows the details of this story, kito can share more details in GNU patche
meeting.
Ya, we can talk tomorrow morning.
Do you guys have a fix for the regressions that showed up over the
weekend?
Either way I'd prefer to go with reverting all this and then taking
Robin's more self-contained fix. If you guys want to do a bigger change
later that's fine, we're just really close to the release and it's not a
good time to risk breaking things. We've only had a few days of a
functioning port over the last week or two, that's already put us behind
on the distro prerelases/RCs so I'm kind of worried something else has
slipped in.
Thanks.
juzhe.zh...@rivai.ai
From: Patrick O'Neill
Date: 2024-04-23 01:20
To: Li, Pan2; Robin Dapp; gcc-patches@gcc.gnu.org
CC: juzhe.zh...@rivai.ai; kito.ch...@gmail.com
Subject: Re: [PATCH v1] RISC-V: Revert RVV wv instructions overlap and xfail
tests
Hi Pan,
I'm not sure I'm following. Did we miss something that should have been
covered? Like only an overlap on the srcs but not the dest?
Are there testcases that fail? If so we should definitely have one.
Can you give some additional information on why these reverts are needed?
+1 to the request for a failing testcase if there is one. Patrick If something
is broken then indeed we should revert it.
Yes, we may need to support these in gcc-15.
... why not just revert everything and xfail all the tests in a
follow up? Your patch is essentially a revert but doesn't look like
it. I'd rather we let a revert be a revert and adjust the tests
separately so it becomes clear.
Sure, will revert b3b2799b872 and then file the patch for the xfail tests.
Pan
