Re: [PATCH] RISC-V: Fix error combine of pred_mov pattern

["Lehua Ding" <lehua.ding@rivai.ai>]

[-- Attachment #1: Type: text/plain, Size: 4465 bytes --]

> But combine doesn't run at -O0.  So something is inconsistent.  I
> certainly believe we need to avoid the mem->mem case, but that's
> independent of combine and affects all optimization levels.


This is an new bug when running all tests after fixing the combine bug.
I understand that maybe I should send a separate patch to fix the problem.
Maybe this problem was exposed after I changed the pattern. I will continue to track it.


> I think we can simplify to just
> !(MEM_P (operands[0]) && MEM_P (operands[1])


> I would have expected those to be handled by the constraints rather than
> the pattern's condition.
Yeh, the condition of the V2 becomes much simpler after split.







------------------ Original ------------------
From:                                                                                                                        "Jeff Law"                                                                                    <gcc-patches@gcc.gnu.org&gt;;
Date:&nbsp;Fri, Aug 11, 2023 11:57 PM
To:&nbsp;"Lehua Ding"<lehua.ding@rivai.ai&gt;;"gcc-patches"<gcc-patches@gcc.gnu.org&gt;;
Cc:&nbsp;"juzhe.zhong"<juzhe.zhong@rivai.ai&gt;;"rdapp.gcc"<rdapp.gcc@gmail.com&gt;;"kito.cheng"<kito.cheng@gmail.com&gt;;"palmer"<palmer@rivosinc.com&gt;;
Subject:&nbsp;Re: [PATCH] RISC-V: Fix error combine of pred_mov pattern



On 8/8/23 21:54, Lehua Ding wrote:
&gt; Hi Jeff,
&gt; 
&gt;&nbsp; &gt; The pattern's operand 0 explicitly allows MEMs as do the constraints.
&gt;&nbsp; &gt;&nbsp;So forcing the operand into a register just seems like it's papering
&gt;&nbsp; &gt;&nbsp;over the real problem.
&gt; 
&gt; The added of force_reg code is address the problem preduced after 
&gt; address the error combine.
&gt; The more restrict condtion of the pattern forbidden mem-&gt;mem pattern 
&gt; which will
&gt; produced in -O0. I think the implementation forgot to do this force_reg 
&gt; operation before
&gt; when doing the intrinis expansion The reason this problem isn't exposed 
&gt; before is because
&gt; the reload pass will converts mem-&gt;mem to mem-&gt;reg; reg-&gt;mem based on 
&gt; the constraint.
So if the core issue if mem-&gt;mem, that is a common thing to avoid.

Basically in the expander you use a force_reg and then have a test like
!(MEM_P (op0) &amp;&amp; MEM_P (op1)) in the define_insn's condition.

But the v1 had a much more complex condition.&nbsp; It looks like that got 
cleaned up in the v2.&nbsp; So I'll need to look at that one more closely.


&gt; 
&gt;&nbsp; &gt; This comment doesn't make sense in conjuction with your earlier details.
&gt;&nbsp; &gt; In particular combine doesn't run at -O0, so your earlier comment that
&gt;&nbsp; &gt; combine creates the problem seems inconsistent with the comment above.
&gt; 
&gt; As the above says, the code addresses the problem which produced
&gt; after addressing the combine problem.
But combine doesn't run at -O0.&nbsp; So something is inconsistent.&nbsp; I 
certainly believe we need to avoid the mem-&gt;mem case, but that's 
independent of combine and affects all optimization levels.


&gt; 
&gt;&nbsp; &gt; Umm, wow.&nbsp; I haven't thought deeply about this, but the complexity of
&gt;&nbsp; &gt; that insn condition is a huge red flag that our operand predicates
&gt;&nbsp; &gt; aren't correct for this pattern.
&gt; 
&gt; This condition is large because the vsetvl info need (compare to scalar 
&gt; mov or *mov<mode&gt;_whole pattern),
&gt; but I think this condition is enough clear to understand. Let me explain 
&gt; briefly.
&gt; 
&gt;&nbsp; &nbsp; &nbsp; (register_operand (operands[0], <MODE&gt;mode) &amp;&amp; MEM_P (operands[3]))
&gt;&nbsp; &nbsp; &nbsp; || (MEM_P (operands[0]) &amp;&amp; register_operand(operands[3], <MODE&gt;mode))
&gt; 
&gt; This two conditons mean allow mem-&gt;reg and reg-&gt;mem pattern.
I think we can simplify to just

 !(MEM_P (operands[0]) &amp;&amp; MEM_P (operands[1])

&gt; 
&gt;&nbsp; &nbsp; &nbsp; (register_operand (operands[0], <MODE&gt;mode) &amp;&amp; 
&gt; satisfies_constraint_Wc1 (operands[1]))
&gt; 
&gt; This condition mean the mask must be all trues for reg-&gt;reg_or_imm 
&gt; pattern since&gt; reg-&gt;reg insn doen't support mask operand.
I would have expected those to be handled by the constraints rather than 
the pattern's condition.

Jeff