on 2024/8/12 21:02, Richard Sandiford wrote:
> "Kewen.Lin" <li...@linux.ibm.com> writes:
>> Hi Richard,
>>
>> Thanks for the comments!
>>
>> on 2024/8/12 16:55, Richard Sandiford wrote:
>>> "Kewen.Lin" <li...@linux.ibm.com> writes:
>>>> 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"))]
>>>
>>> Is it well-formed to have "+" on an operand that is only semantically
>>> an input? df and most passes would consider it to be a pure input
>>> and so would disregard any write that is intended to take place.
>>>
>>> E.g. if we have:
>>>
>>> (set (reg:M R2) (reg:M R1)) ;; R1 dead
>>> (set (reg:M R3) (reg:M R2))
>>>
>>> then it would be valid to change that to:
>>>
>>> (set (reg:M R2) (reg:M R1))
>>> (set (reg:M R3) (reg:M R1))
>>>
>>> without considering the "+" on the input to the first instruction.
>>
>> Good question, I guess the "+" is to match the fact that operand[1]
>> can be both read and written by this insn, operand[1] has to be
>> re-used as the dest register of vector rotate 64bit (doubleword swap).
>> but it'll get swapped back so it's safe if the register is still live
>> (like the above example). For this case that writing to but later
>> restoring, I'm not sure if we can take it as "no write" (strip "+").
>
> Yeah. AIUI, there is no way of satisfying the constraints in a way
> that makes operand 0 overlap operand 1, and:
>
>>
>> ;; The post-reload split requires that we re-permute the source
>> ;; register in case it is still live.
>> (define_split
>> [(set (match_operand:VSX_LE_128 0 "memory_operand")
>> (match_operand:VSX_LE_128 1 "vsx_register_operand"))]
>> "!BYTES_BIG_ENDIAN && TARGET_VSX && reload_completed && !TARGET_P9_VECTOR
>> && !altivec_indexed_or_indirect_operand (operands[0], <MODE>mode)"
>> [(const_int 0)]
>> {
>> rs6000_emit_le_vsx_permute (operands[1], operands[1], <MODE>mode);
>> rs6000_emit_le_vsx_permute (operands[0], operands[1], <MODE>mode);
>> rs6000_emit_le_vsx_permute (operands[1], operands[1], <MODE>mode);
>
> would not work correctly for that kind of overlap in any case.
Guessing you meant the case that both operands are REGs? I may miss
something, but the case here op0 is MEM and op1 is REG, is it still
possible to overlap?
>
> So it looks on the face of it like the pattern would be (more) correct
> without the "+".
>
I just posted a patch to get rid of the "+" at [1], thanks for your
insightful comments!!
[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660238.html
BR,
Kewen
