On Thu, Feb 18, 2021 at 11:58:59AM -0600, will schmidt wrote: > On Wed, 2021-02-03 at 14:37 +0800, Kewen.Lin via Gcc-patches wrote: > > This patch merges the previously approved one[1] and its relied patch > > I don't see the review for [1] in the archives.
I said: Can you make a different testcase perhaps? This patch looks fine otherwise. > > made by Segher here[2], it's to make unsigned int vector init go with > > rldimi to merge two integers instead of shift and ior. > > > > Segher's patch in [2] is required to make the test case pass, > > otherwise the costing for new pseudo-to-pseudo copies and the folding > > with nonzero_bits in combine will make the rl*imi pattern become > > compact and split into ior and shift unexpectedly. > > > > The commit log of Segher's patch describes it in more details: > > > > "An rl*imi is usually written as an IOR of an ASHIFT or similar, and an > > AND of a register with a constant mask. In some cases combine knows > > that that AND doesn't do anything (because all zero bits in that mask > > correspond to bits known to be already zero), and then no pattern > > matches. This patch adds a define_split for such cases. It uses > > nonzero_bits in the condition of the splitter, but does not need it > > afterwards for the instruction to be recognised. This is necessary > > because later passes can see fewer nonzero_bits. > > > > Because it is a splitter, combine will only use it when starting with > > three insns (or more), even though the result is just one. This isn't > > a huge problem in practice, but some possible combinations still won't > > happen." > > > > Bootstrapped/regtested on powerpc64le-linux-gnu P9 and > > powerpc64-linux-gnu P8, also SPEC2017 build/run passed on P9. > > > > Is it ok for trunk? > > > > BR, > > Kewen > > > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-December/562407.html > > [2] https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563526.html > > > > > > gcc/ChangeLog: > > > > 2020-02-03 Segher Boessenkool <seg...@kernel.crashing.org> > > Kewen Lin <li...@gcc.gnu.org> > > > > * config/rs6000/rs6000.md (*rotl<mode>3_insert_3): Renamed to... > > (rotl<mode>3_insert_3): ...this. > > ok > > > (plus_ior_xor): New code_iterator. > > (define_split for GPR rl*imi): New splitter. > > As described above, these two changes appear to identical to what was > posted by Segher in [1]. But with testcase :-) > +/* { dg-final { scan-assembler-not "or" } } */ > > As is, it looks OK to me. Per other reviews i've gotten, you may > get a request to wrap the "or" with \m \M . Right, because it is very easy to end up with "or" as a substring of something else. > Some existing cases with > scan-assembler-not have a leading whitespace/tab qualifier too. > i.e. > /* { dg-final { scan-assembler-not "\[ \t\]or " } } */ Yeah, but the \m in {\mor\M} will do the same already. Thanks, Segher