>> 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.
Just now, I debug and found that the -O0 problem
after repairing error combine was caused by the condition
of pred_mov becoming more strict. Before was
(MEM_P (operands[0]) || MEM_P (operands[3]) || CONST_VECTOR_P
(operands[1])
That is, mem->mem is allowed. This faulty condition causes
two problems at once. One is error combine, the other is to hide
the error pattern with -O0. After correcting the condition with this patch,
I fixed the error combine problem, and also exposed the problem under -O0.
So I think force_reg still needs to be put together with this patch.
------------------ Original ------------------
From:
"Lehua Ding"
<lehua.d...@rivai.ai>;
Date: Sat, Aug 12, 2023 00:30 AM
To: "Jeff
Law"<jeffreya...@gmail.com>;"gcc-patches"<gcc-patches@gcc.gnu.org>;
Cc: "juzhe.zhong"<juzhe.zh...@rivai.ai>;"rdapp.gcc"<rdapp....@gmail.com>;"kito.cheng"<kito.ch...@gmail.com>;"palmer"<pal...@rivosinc.com>;
Subject: Re: [PATCH] RISC-V: Fix error combine of pred_mov pattern
&gt; But combine doesn't run at -O0.&nbsp; So something is
inconsistent.&nbsp; I
&gt; certainly believe we need to avoid the mem-&gt;mem case, but that's
&gt; 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.
&gt; I think we can simplify to just
&gt; !(MEM_P (operands[0]) &amp;&amp; MEM_P (operands[1])
&gt; I would have expected those to be handled by the constraints rather
than
&gt; the pattern's condition.
Yeh, the condition of the V2 becomes much simpler after split.
------------------&nbsp;Original&nbsp;------------------
From:
"Jeff
Law"
<gcc-patches@gcc.gnu.org&gt;;
Date:&nbsp;Fri, Aug 11, 2023 11:57 PM
To:&nbsp;"Lehua
Ding"<lehua.d...@rivai.ai&gt;;"gcc-patches"<gcc-patches@gcc.gnu.org&gt;;
Cc:&nbsp;"juzhe.zhong"<juzhe.zh...@rivai.ai&gt;;"rdapp.gcc"<rdapp....@gmail.com&gt;;"kito.cheng"<kito.ch...@gmail.com&gt;;"palmer"<pal...@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
