> On 1/17/25 7:37 AM, Jin Ma wrote: > >>> Since the parameter vl of XTheadVector does not support immediate > >>> numbers, we need > >>> to put it in the register in advance. That generates the initial code > >>> correctly. > >>> > >>> PR 116593 > >>> > >>> gcc/ChangeLog: > >>> > >>> * config/riscv/riscv-vector-builtins.cc > >>> (function_expander::add_input_operand): > >>> Put immediate for vl to GPR for XTheadVector. > >> > >> Generally both patches look reasonable to me and the change is less > >> invasive > >> than going via spec_restriction. > >> > >> How was this tested? The Rivos CI has already picked it up but please > >> still > >> always specify. Thanks. > > > > I'm not sure what you mean, but I'll do my best to understand. > > If you're referring to how to test the logic of this code to ensure it works > > as intended, it can be quite challenging to explain. Testing this particular > > code is not straightforward. Even without any specific processing, the > > predicate > > "vector_length_operand" still constrains the XTheadVector, and the immediate > > value for VL will be stored in the register. In this sense, the code itself > > may seem redundant. > I suspect Robin is referring to running the GCC testsuite for > regressions. That is standard practice for any patch. > > Essentially build & run make check before and after your patch and > verify there are no new failures. Ideally you'd also verify that your > testcase is fixed :-)
I see, thank you very much for dispelling my doubts. As there is currently no good way to test, perhaps the first patch should be ignored. Let's look at the second patch, which really solves ICE. V4: https://gcc.gnu.org/pipermail/gcc-patches/2025-January/674001.html Thanks, Jin > If you're writing a patch that touches target independent parts of the > compiler, then the bar is even higher. You need to bootstrap & > regression test such patches on one of the primary platforms such as > x86_64, aarch64. > > Jeff
