RE: Re: [PATCH v1] RISC-V: Revert RVV wv instructions overlap and xfail tests

["Li, Pan2" <pan2.li@intel.com>]

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

Thanks Palmer to point this out. Let me share more details about the process.
The revert is not just a git command like git revert commit-id. Consider below scenario.

Day-1: commit-A: rename a attribute.
Day-2: commit-B: other patches reference this renamed attribute but no need to revert.

To follow the suggestion of Robin that keeping the revert commit pure, I followed below step(s)

1. Revert commit-A locally, and address the reference issue.
2. Run fully test to see if any surprise (take 80-90 mins for 64 cores. Sorry again only c/c++ parts, Fortran is missing here).
3. Revert commit-A to upstream (of course it will result in built failure).
4. File patch to address the reference attribute issue.

The other revert almost take similar step(s) but looks some mistake here.
Sorry for that each test result is comparing between code change and upstream, and it will be missed after
we moving on.

> Do you guys have a fix for the regressions that showed up over the 
> weekend?

Understand current we are in the sensitive stage, will take care of the failures ASAP.

Pan

-----Original Message-----
From: Palmer Dabbelt <palmer@dabbelt.com> 
Sent: Tuesday, April 23, 2024 8:43 AM
To: juzhe.zhong@rivai.ai
Cc: Patrick O'Neill <patrick@rivosinc.com>; Li, Pan2 <pan2.li@intel.com>; Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org; Kito Cheng <kito.cheng@gmail.com>
Subject: Re: Re: [PATCH v1] RISC-V: Revert RVV wv instructions overlap and xfail tests

On Mon, 22 Apr 2024 15:07:59 PDT (-0700), juzhe.zhong@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.zhong@rivai.ai
>  
> From: Patrick O'Neill
> Date: 2024-04-23 01:20
> To: Li, Pan2; Robin Dapp; gcc-patches@gcc.gnu.org
> CC: juzhe.zhong@rivai.ai; kito.cheng@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
>