From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 0881C3864839; Mon, 24 Jun 2024 09:51:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0881C3864839 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=linux.ibm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linux.ibm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 0881C3864839 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.158.5 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1719222677; cv=none; b=DmnmQ1rn58BkMdfLkJE3GgvA7voK6yPO4bimSBIiUmWOyyxJSbmoTMteu/vmi2s8uFzQ2MzMUVBRL3f2re2nZ17TjFWJHmWHZ67vLMKU33t2maHv+4HewZyhQjfOJJ2XL9nyihsXOcNvOTlN7p2a1h9Kvga2iSdHiDI07E9IHjg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1719222677; c=relaxed/simple; bh=GH7iPXv+SQ641vNi7O0FD+x9vyfiYUV1cAb8JiKycMY=; h=DKIM-Signature:Message-ID:Date:Subject:From:To:MIME-Version; b=stNpoOZ0ztNy72CKR4DL+X/GgcfcH1vos17PVOrKBfXH4DMD2PH/DkvTNge9wMK/S/LNQ8yb7Q2Xhbqzm6SMDOUHV8bnB5cooP9FAjJoXbf8sQBNE4jYUQHRJyXd1Ltylc682NAE5V4yiQP15P1OzDoO6j29S3f7yPUikNXHtNM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0353725.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 45O3x7d4030504; Mon, 24 Jun 2024 06:23:54 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h= message-id:date:subject:from:to:cc:references:in-reply-to :content-type:content-transfer-encoding:mime-version; s=pp1; bh= 8I9QBMfTpOIt/0OBexPA4euDVKBfHwcmzgjzLI5OtwA=; b=lCdTBb1Rd4n9SlJS N1MsaDpDYNI6r+gG9E3FD3mxz+CtsF8n7G/i3e8gSFNsNHWmdG6VVB+AmnK5hNRf dGtGJ1mtuTiS6Pa08h8yRneVJJfl9miE+gsmQkkv18efrAa5gm9I8vs7PZhX5JqW qXZhSbYv/C/QAICrA1WK+zTOldbm3WQ+/ZKoJvoZP8RyVXMN01r4tA5Fl4XqsKAl XJxQRFb2pWlJiHL43UVFclUbZTkNWFq83T0hEyskLG6FxjHpQL0PhCeRdyNnFTjU Ko53blm60PSO0NE+64UZIxUAnRzACWir821ilbxmf6qsjTyv0Ky4vHHQSAzhT7Y0 kJ9Nlg== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3yy1c48aea-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 24 Jun 2024 06:23:53 +0000 (GMT) Received: from m0353725.ppops.net (m0353725.ppops.net [127.0.0.1]) by pps.reinject (8.18.0.8/8.18.0.8) with ESMTP id 45O6Nrv3027383; Mon, 24 Jun 2024 06:23:53 GMT Received: from ppma11.dal12v.mail.ibm.com (db.9e.1632.ip4.static.sl-reverse.com [50.22.158.219]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3yy1c48ae7-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 24 Jun 2024 06:23:53 +0000 (GMT) Received: from pps.filterd (ppma11.dal12v.mail.ibm.com [127.0.0.1]) by ppma11.dal12v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 45O5dPZp000411; Mon, 24 Jun 2024 06:23:52 GMT Received: from smtprelay06.fra02v.mail.ibm.com ([9.218.2.230]) by ppma11.dal12v.mail.ibm.com (PPS) with ESMTPS id 3yxbn2wdmg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 24 Jun 2024 06:23:52 +0000 Received: from smtpav07.fra02v.mail.ibm.com (smtpav07.fra02v.mail.ibm.com [10.20.54.106]) by smtprelay06.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 45O6NmgI35127872 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 24 Jun 2024 06:23:50 GMT Received: from smtpav07.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id C265A20040; Mon, 24 Jun 2024 06:23:48 +0000 (GMT) Received: from smtpav07.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id B638C2004B; Mon, 24 Jun 2024 06:23:46 +0000 (GMT) Received: from [9.197.227.211] (unknown [9.197.227.211]) by smtpav07.fra02v.mail.ibm.com (Postfix) with ESMTP; Mon, 24 Jun 2024 06:23:46 +0000 (GMT) Message-ID: <0dbc2998-1948-3683-18f2-23d6d5267480@linux.ibm.com> Date: Mon, 24 Jun 2024 14:23:45 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [PATCH 10/52] jit: Replace uses of {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE Content-Language: en-US From: "Kewen.Lin" To: David Malcolm Cc: gcc-patches@gcc.gnu.org, jit@gcc.gnu.org, Richard Biener , Joseph Myers References: <7c9b2ed53cee1c8c7d5b47abbf963acc2bf5a62e.1717134752.git.linkw@linux.ibm.com> <24355f0ad35a6ce7619be47c0f6cd69624d250cf.camel@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: LCzw8ed1EasaQ5MoqbXgMmyAM6P9DfXj X-Proofpoint-GUID: f6Bun_SjYl00-0LxyACTmNKoDWyksM6E Content-Transfer-Encoding: 8bit X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16 definitions=2024-06-24_05,2024-06-21_01,2024-05-17_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 priorityscore=1501 mlxscore=0 bulkscore=0 phishscore=0 suspectscore=0 spamscore=0 adultscore=0 lowpriorityscore=0 mlxlogscore=999 clxscore=1015 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.19.0-2406140001 definitions=main-2406240050 X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Dave, May I ask if you still have some concerns on this patch with some replies to your previous questions? BR, Kewen on 2024/6/14 10:16, Kewen.Lin wrote: > Hi David, > > on 2024/6/13 21:44, David Malcolm wrote: >> On Sun, 2024-06-02 at 22:01 -0500, Kewen Lin wrote: >>> Joseph pointed out "floating types should have their mode, >>> not a poorly defined precision value" in the discussion[1], >>> as he and Richi suggested, the existing macros >>> {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE will be replaced with a >>> hook mode_for_floating_type.  Unlike the other FEs, for the >>> uses in recording::memento_of_get_type::get_size, since >>> {float,{,long_}double}_type_node haven't been initialized >>> yet, this is to replace {FLOAT,{,LONG_}DOUBLE}_TYPE_SIZE >>> with calling hook targetm.c.mode_for_floating_type. >>> >>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-May/651209.html >>> >>> gcc/jit/ChangeLog: >>> >>>         * jit-recording.cc >>> (recording::memento_of_get_type::get_size): Update >>>         macros {FLOAT,DOUBLE,LONG_DOUBLE}_TYPE_SIZE by calling >>>         targetm.c.mode_for_floating_type with >>>         TI_{FLOAT,DOUBLE,LONG_DOUBLE}_TYPE. >>> --- >>>  gcc/jit/jit-recording.cc | 12 ++++++++---- >>>  1 file changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc >>> index 68a2e860c1f..7719b898e57 100644 >>> --- a/gcc/jit/jit-recording.cc >>> +++ b/gcc/jit/jit-recording.cc >>> @@ -21,7 +21,7 @@ along with GCC; see the file COPYING3.  If not see >>>  #include "config.h" >>>  #include "system.h" >>>  #include "coretypes.h" >>> -#include "tm.h" >>> +#include "target.h" >>>  #include "pretty-print.h" >>>  #include "toplev.h" >>>   >>> @@ -2353,6 +2353,7 @@ size_t >>>  recording::memento_of_get_type::get_size () >>>  { >>>    int size; >>> +  machine_mode m; >>>    switch (m_kind) >>>      { >>>      case GCC_JIT_TYPE_VOID: >>> @@ -2399,13 +2400,16 @@ recording::memento_of_get_type::get_size () >>>        size = 128; >>>        break; >>>      case GCC_JIT_TYPE_FLOAT: >>> -      size = FLOAT_TYPE_SIZE; >>> +      m = targetm.c.mode_for_floating_type (TI_FLOAT_TYPE); >>> +      size = GET_MODE_PRECISION (m).to_constant (); >>>        break; >>>      case GCC_JIT_TYPE_DOUBLE: >>> -      size = DOUBLE_TYPE_SIZE; >>> +      m = targetm.c.mode_for_floating_type (TI_DOUBLE_TYPE); >>> +      size = GET_MODE_PRECISION (m).to_constant (); >>>        break; >>>      case GCC_JIT_TYPE_LONG_DOUBLE: >>> -      size = LONG_DOUBLE_TYPE_SIZE; >>> +      m = targetm.c.mode_for_floating_type (TI_LONG_DOUBLE_TYPE); >>> +      size = GET_MODE_PRECISION (m).to_constant (); >>>        break; >>>      case GCC_JIT_TYPE_SIZE_T: >>>        size = MAX_BITS_PER_WORD; >> >> [CCing jit mailing list] >> >> Thanks for the patch; sorry for the delay in responding. >> >> Did your testing include jit? Note that --enable-languages=all does >> *not* include it (due to it needing --enable-host-shared). > > Thanks for the hints! Yes, as noted in the cover letter, I did test jit. > Initially I used TYPE_PRECISION ({float,{long_,}double_type_node) to > replace these just like what I proposed for the other FE changes, but the > testing showed some failures on test-combination.c etc., by looking into > them, I realized that this call recording::memento_of_get_type::get_size > can happen before when we set up those type nodes. Then I had to use the > current approach with the new hook, it made all failures gone (no > regressions). btw, test result comparison showed some more lines with > "NA->PASS: test-threads.c.exe", since it's positive, I didn't look into > it. > >> >> The jit::recording code runs *very* early - before toplev::main. For >> example, a call to gcc_jit_type_get_size can trigger the above code >> path before toplev::main has run. >> >> target.h says each target should have a: >> >> struct gcc_target targetm = TARGET_INITIALIZER; >> >> Has targetm.c.mode_for_floating_type been initialized enough by that >> static initialization? > > It depends on how to define "enough". The hook has been initialized > as you pointed out, I just debugged it and confirmed target specific > hook was called as expected (rs6000_c_mode_for_floating_type on Power) > when this jit::recording function gets called. If "enough" refers to > something like command line options, it's not ready. > >> Could the mode_for_floating_type hook be >> relying on some target-specific dynamic initialization that hasn't run >> yet? (e.g. taking account of command-line options?) >> > > Yes, it could. Like rs6000 port, the hook checks rs6000_long_double_type_size > for long double (it's related to command line option -mlong-double-x) and > some other targets like i386, also would like to check TARGET_LONG_DOUBLE_64 > and TARGET_LONG_DOUBLE_128. But I think it isn't worse than before, without > this change (with the previous macro), we used to define the macro with > the things related to this command line options, which are still not ready. > > #define LONG_DOUBLE_TYPE_SIZE rs6000_long_double_type_size > > I debugged the code, jit::recording will see rs6000_long_double_type_size > with the static initialized value zero, it means that the function > recording::memento_of_get_type::get_size would get zero byte size for the > type (I assume that it's unexpected for the code?). With this new hook, > although it can provide not exact type size (can be off from the one > specified by command line), it returns a reasonable size (comparing with > the zero size). From this perspective, it's slightly better? > > + if (ti == TI_LONG_DOUBLE_TYPE) > + return rs6000_long_double_type_size == FLOAT_PRECISION_TFmode ? TFmode > + : DFmode; > > BR, > Kewen >