On 3/3/21 1:42 PM, Ilya Leoshkevich wrote:
> On Wed, 2021-03-03 at 21:26 +0100, Ilya Leoshkevich via Gcc-patches
> wrote:
>> On Wed, 2021-03-03 at 13:02 -0700, Jeff Law wrote:
>>>
>>> On 3/2/21 4:50 PM, Ilya Leoshkevich via Gcc-patches wrote:
>>>> Hello,
>>>>
>>>> I would like to ping the following patch:
>>>>
>>>> Add input_modes parameter to TARGET_MD_ASM_ADJUST hook
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/562898.html
>>>>
>>>> It is needed for the following regression fix:
>>>>
>>>> IBM Z: Fix usage of "f" constraint with long doubles
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564380.html
>>>>
>>>>
>>>> Jakub, who would be the right person to review this change? I've
>>>> decided to ask you, since `git shortlog -ns gcc/cfgexpand.c` shows
>>>> that
>>>> you deal with this code a lot.
>>>>
>>>> Best regards,
>>>> Ilya
>>>>
>>>>
>>>>
>>>>
>>>> If TARGET_MD_ASM_ADJUST changes a mode of an input operand (which
>>>> should be ok as long as the hook itself as well as after_md_seq
>>>> make up
>>>> for it), input_mode will contain stale information.
>>>>
>>>> It might be tempting to fix this by removing input_mode altogether
>>>> and
>>>> just using GET_MODE (), but this will not work correctly with
>>>> constants.
>>>> So add input_modes parameter and document that it should be updated
>>>> whenever inputs parameter is updated.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2021-01-05 Ilya Leoshkevich <i...@linux.ibm.com>
>>>>
>>>> * cfgexpand.c (expand_asm_loc): Pass new parameter.
>>>> (expand_asm_stmt): Likewise.
>>>> * config/arm/aarch-common-protos.h (arm_md_asm_adjust): Add
>>>> new
>>>> parameter.
>>>> * config/arm/aarch-common.c (arm_md_asm_adjust): Likewise.
>>>> * config/arm/arm.c (thumb1_md_asm_adjust): Likewise.
>>>> * config/cris/cris.c (cris_md_asm_adjust): Likewise.
>>>> * config/i386/i386.c (ix86_md_asm_adjust): Likewise.
>>>> * config/mn10300/mn10300.c (mn10300_md_asm_adjust):
>>>> Likewise.
>>>> * config/nds32/nds32.c (nds32_md_asm_adjust): Likewise.
>>>> * config/pdp11/pdp11.c (pdp11_md_asm_adjust): Likewise.
>>>> * config/rs6000/rs6000.c (rs6000_md_asm_adjust): Likewise.
>>>> * config/vax/vax.c (vax_md_asm_adjust): Likewise.
>>>> * config/visium/visium.c (visium_md_asm_adjust): Likewise.
>>>> * target.def (md_asm_adjust): Likewise.
>>> Ugh. A couple questions
>>> Are there any cases where you're going to want to change modes for
>>> arguments that were constants? I'm a bit surprised that we don't
>>> have
>>> a mode for constants for the cases that we care about. Presumably we
>>> can get a (modeless) CONST_INT here and we're not restricted to
>>> CONST_DOUBLE and friends (which have modes).
>> Yes, this might happen. For example, here:
>>
>> asm("sqxbr\t%0,%1" : "=f"(res) : "f"(0x1.0000000000001p+0L));
>>
>> the (const_double) and the corresponding operand will initially have
>> the mode TFmode. s390_md_asm_adjust () will add a conversion from
>> TFmode to FPRX2mode and change the argument accordingly.
> Just to be more precise: the mode of the (const_double) itself will not
> change. Here is the resulting RTL for the asm statement above:
>
> # s390_md_asm_adjust () step 1: put the (const_double) operand into a
> # new (reg) with the same mode
> (insn (set (reg:TF 63)
> (const_double:TF ...)))
OK, but you still need to know the constant's original mode so that you
can get the right mode on the insn above and in the SUBREG_REG for the
insn in step #2. Those cases were exposed by the testsuite.
I'd originally wondered if we could just not adjust in the CONST_INT
case, but I think I was mis-understanding the implementation of
s390_md_asm_adjust in an important way.
I also think I focused too much on expand_asm_loc where it doesn't look
like we read input_mode after the call to MD_ASM_ADJUST. But we
certainly do in expand_asm_stmt.
OK for the trunk.
Thanks,
jeff
