Hi Vladimir,
on 2024/8/10 01:47, Vladimir Makarov wrote:
>
> On 8/9/24 05:49, Kewen.Lin wrote:
>> Hi,
>>
>> Commit r15-2084 exposes one ICE in LRA. Firstly, before
>> r15-2084 KFmode has 126 bit precision while V1TImode has 128
>> bit precision, so the subreg (subreg:V1TI (reg:KF 131) 0) is
>> paradoxical_subreg_p, which stops some passes from doing
>> some optimization. After r15-2084, KFmode has the same mode
>> precision as V1TImode, passes are able to optimize more, but
>> it causes this ICE in LRA as described below:
>>
>> For insn 106 (set (mem:V1TI ...) (subreg:V1TI (reg:KF 133) 0)),
>> which matches pattern
>>
>> (define_insn "*vsx_le_perm_store_<mode>"
>> [(set (match_operand:VSX_LE_128 0 "memory_operand" "=Z,Q")
>> (match_operand:VSX_LE_128 1 "vsx_register_operand" "+wa,r"))]
>> "!BYTES_BIG_ENDIAN && TARGET_VSX && !TARGET_P9_VECTOR
>> && !altivec_indexed_or_indirect_operand (operands[0], <MODE>mode)"
>> "@
>> #
>> #"
>> [(set_attr "type" "vecstore,store")
>> (set_attr "length" "12,8")
>> (set_attr "isa" "<VSisa>,*")])
>> LRA makes equivalence substitution on r133 with const double
>> (const_double:KF 0.0), selects alternative 0 and fixes up
>> operand 1 for constraint "wa", because operand 1 is OP_INOUT,
>> so it considers assigning back to it as well, that is:
>>
>> lra_emit_move (type == OP_INOUT ? copy_rtx (old) : old, new_reg);
>>
>> But because old has been changed to const_double in equivalence
>> substitution, the move is actually assigning to const_double,
>> which is invalid and cause ICE.
>>
>> Considering reg:KF 133 is equivalent with (const_double:KF 0.0)
>> even though this operand is OP_INOUT, IMHO there should not be
>> any following uses of reg:KF 133, otherwise it doesn't have the
>> chance to be equivalent to (const_double:KF 0.0). From this
>> perspective, I think we can guard the lra_emit_move with
>> nonimmediate_operand to exclude such case.
>>
>> Does it sound reasonable?
> Yes.
>> btw, I also tried with disallowing equivalence substitution with
>> CONSTANT_P value if the corresponding operand is OP_INOUT or
>> OP_OUT, it can also fix this issue, but with more thinking it
>> seems not necessary to stop such substitution if we can handle it
>> later as above.
>>
>> Bootstrapped and regtested on x86_64-redhat-linux and
>> powerpc64{,le}-linux-gnu.
>>
> Thank you for the good explanation of the problem. The patch is ok for me.
> It would be nice to add a comment before `nonimmediate_operand` that `old`
> can be an equivalent constant and we chose insn alternative before the
> equivalent substitution.
Thanks for your comments!! If I read the code right, the function chooses
insn alternative after equivalence substitution, the related code is:
4207 if (lra_dump_file != NULL)
4208 {
4209 fprintf (lra_dump_file,
4210 "Changing pseudo %d in operand %i of insn %u on
equiv ",
4211 REGNO (old), i, INSN_UID (curr_insn));
4212 dump_value_slim (lra_dump_file, subst, 1);
4213 fprintf (lra_dump_file, "\n");
4214 }
4215 op_change_p = change_p = true;
4216 }
4217 if (simplify_operand_subreg (i, GET_MODE (old)) || op_change_p)
4218 {
4219 change_p = true;
4220 lra_update_dup (curr_id, i);
4221 }
4411 fprintf (lra_dump_file, " Choosing alt %d in insn %u:",
4412 goal_alt_number, INSN_UID (curr_insn));
4413 print_curr_insn_alt (goal_alt_number);
so I just added a comment as you suggested but stripping "and ...":
diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
index 92b343fa99a..f355c6c6168 100644
--- a/gcc/lra-constraints.cc
+++ b/gcc/lra-constraints.cc
@@ -4742,7 +4742,9 @@ curr_insn_transform (bool check_only_p)
}
*loc = new_reg;
if (type != OP_IN
- && find_reg_note (curr_insn, REG_UNUSED, old) == NULL_RTX)
+ && find_reg_note (curr_insn, REG_UNUSED, old) == NULL_RTX
+ /* OLD can be an equivalent constant here. */
+ && nonimmediate_operand (old, GET_MODE (old)))
{
start_sequence ();
lra_emit_move (type == OP_INOUT ? copy_rtx (old) : old, new_reg);
Does it look good to you? Or did I miss something here?
Thanks again!
BR,
Kewen
>
> Thank you for fixing the PR.
>
>> PR rtl-optimization/116170
>>
>> gcc/ChangeLog:
>>
>> * lra-constraints.cc (curr_insn_transform): Don't emit move back to
>> old operand if it's nonimmediate_operand.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.target/powerpc/pr116170.c: New test.
>> ---
>> gcc/lra-constraints.cc | 3 ++-
>> gcc/testsuite/gcc.target/powerpc/pr116170.c | 18 ++++++++++++++++++
>> 2 files changed, 20 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr116170.c
>>
>> diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
>> index 92b343fa99a..024c85c87d9 100644
>> --- a/gcc/lra-constraints.cc
>> +++ b/gcc/lra-constraints.cc
>> @@ -4742,7 +4742,8 @@ curr_insn_transform (bool check_only_p)
>> }
>> *loc = new_reg;
>> if (type != OP_IN
>> - && find_reg_note (curr_insn, REG_UNUSED, old) == NULL_RTX)
>> + && find_reg_note (curr_insn, REG_UNUSED, old) == NULL_RTX
>> + && nonimmediate_operand (old, GET_MODE (old)))
>> {
>> start_sequence ();
>> lra_emit_move (type == OP_INOUT ? copy_rtx (old) : old, new_reg);
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr116170.c
>> b/gcc/testsuite/gcc.target/powerpc/pr116170.c
>> new file mode 100644
>> index 00000000000..6f6ca0f1ae9
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr116170.c
>> @@ -0,0 +1,18 @@
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target ppc_float128_sw } */
>> +/* { dg-options "-mdejagnu-cpu=power8 -O2 -fstack-protector-strong
>> -ffloat-store" } */
>> +
>> +/* Verify there is no ICE. */
>> +
>> +int a, d;
>> +_Float128 b, c;
>> +void
>> +e ()
>> +{
>> + int f = 0;
>> + if (a)
>> + if (b || c)
>> + f = 1;
>> + if (d)
>> + e (f ? 0 : b);
>> +}
>> --
>> 2.43.5
>>
>
