On Fri, Jul 17, 2026 at 10:45 AM Jeffrey Law <[email protected]> wrote: > > > > On 7/14/2026 8:19 AM, Dusan Stojkovic wrote: > > > Note that canonicalization is driven more by the SSA_NAME_VERSION and IL > > > structure than types and getting things in the right form to expose this > > > case may be hard. > > > > > > One approach I've taken through the years is to sketch out such a > > > pattern and have it abort() when it matches. Then I run a large > > > codebase through that compiler to see if I can get it to trigger that > > > abort. Much like basic pattern development, I often start with a > > > simplistic, over-matching pattern and refine. That makes it less likely > > > that a logical big preventing a match makes me conclude the pattern is > > > unnecessary/worthless. Assuming that process finds a real trigger, I > > > then reduce it and include going forward. > > > > > > It's not critical for this patch IMHO, but perhaps an avenue to explore > > > in the future. Based on Andrea's comments, I suspect we're in agreement > > > on that. > > > > As I mentioned, usually I just try different combinations of the same > > testcase to see if it behaves differently, but your suggestion makes a > > lot of sense too, though I haven't tried it for this version of the > > patch. > > > > > Don't the tests in the IF clause result in type <= ltype0 <= ltype1, yet > > > in the WITH clause we're assigning largertype to ltype0. Something > > > seems wrong with that. > > > > Yes, and now that you mention it largertype does seem redundant. I > > removed it in this version and used @4 (associated with ltype1) instead > > of @1. > > > > > Your scan-assembler-times tests are a bit inconsistent in that they > > > don't anchor with a tab before or after the opcode with any > > > consistency. We're not terribly good about this in the testsuite, so > > > it's not a major problem, though we should try to be consistent within > > > any given test if we can. > > > > I added the tab at first to differentiate between andi and and, so now I > > have it on all scan-assembler-times tests for consistency. > > Also, I tweaked the values in the testcase b a bit because for some > > reason the assembly generated for rv32 wasn't branchless. It is > > branchless now. > > > > > Overall I think you're on the right track here. As I mentioned in the > > > call last week, it's great to see this coming together after years of > > > banging our heads on the wall. > > > > > > Jeff > > > > Glad to help, > > Dusan > > > > 2026-07-14 Dusan Stojkovic <[email protected]> > > > > PR target/123883 > > > > gcc/ChangeLog: > > > > * match.pd: New pattern. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/riscv/pr123883.c: New test. > > Co-authored-by: Jeff Law <[email protected]> > This looks good. I guess the only question is whether or not to add the > :c to the bit_and to allow it to match both forms. I realize that > without a testcase we're not going to test the alternate form, but > looking at the pattern I don't see any real reason for the other form to > show up. So I'm going to include the :c.
The easiest way to get a testcase for both forms is a gimple based testcase. Where you have one where the higher version is the larger type and then have a function where the lower version is the larger type. I would write a testcase for this (I have also written a C testcase before for a missing :c too) but I am on vacation. r14-3842-gb59e9de990a17b and r14-3832-g190cf0ce8f4c14 are examples where I found the missing `:c` before. r14-3832-g190cf0ce8f4c14's test2/test2_ how you can get the ssa name versions in reverse order; since ssa renaming is dominating based the first referenced will get a lower number. Thanks, Andrea > > I'll do another bootstrap & regression test on x86-64 since mine ran > without the :c. I'll also do spot testing of riscv32-elf and > riscv64-elf. Assuming that's clean, I'll push it later today. > > jeff > >
