public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Robin Dapp <rdapp.gcc@gmail.com>
To: pan2.li@intel.com, gcc-patches@gcc.gnu.org
Cc: rdapp.gcc@gmail.com, juzhe.zhong@rivai.ai, kito.cheng@sifive.com,
	yanzhang.wang@intel.com
Subject: Re: [PATCH v8] RISC-V: Support CALL for RVV floating-point dynamic rounding
Date: Fri, 28 Jul 2023 12:05:21 +0200	[thread overview]
Message-ID: <c10ca37c-fbc7-b812-b04a-7e7a6d9a7960@gmail.com> (raw)
In-Reply-To: <20230728011521.3280522-1-pan2.li@intel.com>

Hi Pan,

thanks for your patience and your work.  Apart from my general doubt
whether mode-changing intrinsics are a good idea, I don't have other
remarks that need fixing.  What I mentioned before:

 - Handling of asms wouldn't be a huge change.  It can be done
 in a follow-up patch of course but should be done eventually.

 - The code is still rather difficult to follow because we diverge
 from the usual mode-switching semantics e.g. in that we emit insns
 in mode_needed as well as in mode_set.  I would have preferred
 to stay close to the regular usage, document where and why we need
 to do something different and suggest future middle-end improvements
 to solve this more elegantly.

 - I hope non-local control flow like setjmp/longjmp, sibcall
 optimization and maybe others work fine.  I didn't see a reason
 why not but I haven't checked very closely either.

 - We can probably get away with not annotating every call with
 an FRM clobber because there isn't any pass that would make use
 of that anyway?


As to my general qualm, independent of this patch, quickly
summarized again one last time (the problem was latent before this
specific patch anyway):

I would prefer not to have mode-changing intrinsics at all but
have users call fesetround explicitly.  That way the exact point
where the rounding mode is changed would be obvious and not
subject to optimization as well as caching/backing up.
If at all necessary I would have preferred the LLVM way of
backing up, setting new mode, performing the instruction
and restoring directly after.
If the initial intent of mode-changing intrinsics was to give
users more control, I don't believe we achieve this by the "lazy"
restore mechanism which is rather an obfuscation.

Pardon my frankness but the whole mode-changing thing feels to me
like just getting a feature out of the door to solve "something"
/appease users than a well thought-out feature.  It doesn't even
seem clear if this optimization is worthwhile when changing the
rounding mode is prohibitively slow anyway.

That said, if the current status is what the majority of
contributors can live with, I'm not going to stand in the way,
but I'd ask Kito or somebody else to give the final OK.

Regards
 Robin

  reply	other threads:[~2023-07-28 10:05 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-19  3:28 [PATCH v1] " pan2.li
2023-07-19  3:31 ` juzhe.zhong
2023-07-19  6:30   ` Li, Pan2
2023-07-20  6:43 ` [PATCH v4] " pan2.li
2023-07-20  6:47   ` juzhe.zhong
2023-07-21  3:11   ` juzhe.zhong
2023-07-21  6:44     ` Li, Pan2
2023-07-23 13:11 ` [PATCH v5] " pan2.li
2023-07-24  0:53   ` juzhe.zhong
2023-07-24  1:51     ` Li, Pan2
2023-07-24  2:45       ` Li, Pan2
2023-07-24  2:42 ` [PATCH v6] " pan2.li
2023-07-24 10:28   ` Robin Dapp
2023-07-24 11:59     ` Li, Pan2
2023-07-24 12:03       ` Li, Pan2
2023-07-25  5:51 ` [PATCH v7] " pan2.li
2023-07-25  6:07   ` Li, Pan2
2023-07-25  8:38     ` Robin Dapp
2023-07-25 11:53       ` Li, Pan2
2023-07-25 13:23         ` Kito Cheng
2023-07-25 14:12           ` Robin Dapp
2023-07-26 13:08             ` Robin Dapp
2023-07-26 21:40               ` Jeff Law
2023-07-26 22:21                 ` 钟居哲
2023-07-26 22:46                   ` Jeff Law
2023-07-26 22:56                     ` 钟居哲
2023-07-27  1:38                       ` Li, Pan2
2023-07-27  8:19                         ` Li, Pan2
2023-07-27  2:09               ` Li, Pan2
2023-07-27  7:25                 ` Robin Dapp
2023-07-27  8:26                   ` Li, Pan2
2023-07-27  8:41                     ` Robin Dapp
2023-07-27 10:27                       ` Li, Pan2
     [not found]             ` <63471C6E126E44CF+D1CEA4C9-0050-43CD-8DE3-26EBD7AEE6DA@rivai.ai>
2023-07-26 13:35               ` Li, Pan2
2023-07-26 13:43                 ` Li, Pan2
2023-07-26 13:46               ` Robin Dapp
2023-07-26 13:57                 ` Kito Cheng
2023-07-26 14:05                   ` Kito Cheng
2023-07-26 14:10                     ` Robin Dapp
2023-07-26 14:18                 ` 钟居哲
2023-07-26 14:30                   ` Li, Pan2
2023-07-26 15:34                     ` Kito Cheng
2023-07-26 16:00                       ` Palmer Dabbelt
2023-07-26 21:01                     ` Robin Dapp
2023-07-28  1:15 ` [PATCH v8] " pan2.li
2023-07-28 10:05   ` Robin Dapp [this message]
2023-07-28 12:34     ` Li, Pan2
2023-08-01  7:50       ` Kito Cheng
2023-08-01  8:00         ` Li, Pan2

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c10ca37c-fbc7-b812-b04a-7e7a6d9a7960@gmail.com \
    --to=rdapp.gcc@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=juzhe.zhong@rivai.ai \
    --cc=kito.cheng@sifive.com \
    --cc=pan2.li@intel.com \
    --cc=yanzhang.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).