Hi Jeff, Thank you for your response. I have attempted to construct a regression test case where the old HISI definition causes a failure (e.g., incorrect code generation or ICE) while the new HX definition works correctly. But I was unable to construct such a case. However, the change to HX is still technically necessary for correctness. It strictly enforces the hardware constraint that pack only operates on XLEN/2 bits. Given this, I consider this is a semantic cleanup to align strictly with the hardware specification , even without a specific failing testcase. Is it okay to merge it?
[email protected] From: Jeffrey Law Date: 2026-06-10 23:52 To: [email protected]; Jeffrey Law; [email protected] CC: [email protected]; [email protected]; [email protected]; [email protected]; pan2.li; [email protected]; [email protected] Subject: Re: [PATCH] Fix data type iterator of pack rs reg On 6/10/2026 2:49 AM, [email protected] wrote: > Hi Jeff and Pan, > > I'd like to elaborate on the motivation for using the HX iterator > instead of HISI. > According to the RISC-V specification, the pack instruction strictly > operates on XLEN/2 bits. The HISI iterator unconditionally expands to > [HI, SI], which means on RV64 targets, GCC would unnecessarily > generate and maintain dead RTL matching templates for HImode that can > never be matched by the hardware. > The HX iterator ([(HI "!TARGET_64BIT") (SI "TARGET_64BIT")]) perfectly > aligns with the hardware semantics. It ensures that: > On RV32, we only match HImode inputs. > On RV64, we only match SImode inputs. > This makes the RTL expansion much more efficient and strictly correct. Right. Totally agreed. > Regarding the test cases, I noticed that there are already existing > test cases (zbkb32.c and zbkb64.c) in the testsuite. I have run them > locally and verified that this change has no negative impact on them. > Since the existing tests already cover the pack instruction generation > and pass successfully, I believe they are sufficient to verify the > correctness of this patch. But if those zbkb{32,64}.c tests were already running correctly, then we don't have a testcase which shows why you made this change. While I completely understand and agree that the change is technically correct, what we're looking for is a testcase which shows GCC not behaving correctly before your patch and behaving correctly after your patch. It's not a hard requirement, but we do try to cover this kind of change with a testcase as often as possible. Jeff
