From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com [IPv6:2607:f8b0:4864:20::102d]) by sourceware.org (Postfix) with ESMTPS id AE5363858D32 for ; Mon, 7 Nov 2022 19:01:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AE5363858D32 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dabbelt.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dabbelt.com Received: by mail-pj1-x102d.google.com with SMTP id h14so11434344pjv.4 for ; Mon, 07 Nov 2022 11:01:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dabbelt-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:from:to:cc:subject:date:message-id :reply-to; bh=IOe/XHO3ds2W1srqY3iwv0WamVsVY/SUJuYi69RlC2o=; b=qkgF/G8ObOHq1zAKhuIIJ7/GciudMbbbz4fjjsS0NptsmBs9p5VzRmsNm5toy/ohg1 eu9AZn3nuBbPcqu/zGaCWtNt4ChdVKUl7CZzDmhh8H1kKEKs8ctbkrtXQ84AKeWWEXKV RW2CWDLV5eCnZ3VU3XGsnBCRU+aWEOvl5woyLDEyqi8Z9m1xjlbI8oLz3PtbGBGp/5fn jnLkyOFt7pZZzBu90+NTrJNgtRU/MlL1MgmJB6lgSKM+jtj1lncnZsQfDRjyKiR7Xy4o Y8N7cKC+cDQN5VJBkCPy7eM9I1YDstF+zpLTGc6ljwilkJtGyvYMJOXYpt75BnEMTEaG spLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=IOe/XHO3ds2W1srqY3iwv0WamVsVY/SUJuYi69RlC2o=; b=raRoLBZxKBW3jWXemhmwYW57tmcjamWcrmTX51jmf1E/sa8ySLkaR4sxpmra8Iw7ho LnxicbAXHJbRgFD0CZV1jGfE7Rt7BehmkTob7Hhp1DcbnALjxIA1tMg78AnfAV5iC9mc tDnbpUSd9NkQUv0+fd4QLV3D4RJvA0A221y4KjCKiJZd4TGa4OAJXXdfr3YuCX/vWxZ2 Ds/GtdQ3Rty2MtKuweC9JAgLqWh3eXV3084+vT++eD/4K9i6lwssn53xP+MUVIPrKiNJ ekCvXH/3Ov84Jx9HJNP4w7CysrfpbVSRu+b/V7sLi/b8/H1EDwWrMkr3Tc1VNYdSALVU GBxw== X-Gm-Message-State: ACrzQf1JYiSvg6OPgPdaIAeelNGNFwj0Up65vcRd7WPwiLH45b4I1iTH NjyLZZnqX58zHYZ8wTOKcjGGRg== X-Google-Smtp-Source: AMsMyM48hwIwrromRVShbS3v+OMamBZBlFdWM8VEMYyGFBLIF2Av7rpb6aUroHOkQm+fwYKhP201nQ== X-Received: by 2002:a17:902:bcc1:b0:187:31da:494a with SMTP id o1-20020a170902bcc100b0018731da494amr37350343pls.121.1667847665236; Mon, 07 Nov 2022 11:01:05 -0800 (PST) Received: from localhost ([50.221.140.188]) by smtp.gmail.com with ESMTPSA id u130-20020a627988000000b0056bb99db338sm5002643pfc.175.2022.11.07.11.01.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 07 Nov 2022 11:01:04 -0800 (PST) Date: Mon, 07 Nov 2022 11:01:04 -0800 (PST) X-Google-Original-Date: Mon, 07 Nov 2022 11:00:47 PST (-0800) Subject: Re: [RFC] RISC-V: Add profile supports. In-Reply-To: CC: kito.cheng@sifive.com, wuwei2016@iscas.ac.cn, gcc-patches@gcc.gnu.org, philipp.tomsich@vrull.eu, jiawei@iscas.ac.cn From: Palmer Dabbelt To: christoph.muellner@vrull.eu Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,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: On Thu, 03 Nov 2022 12:11:31 PDT (-0700), christoph.muellner@vrull.eu wrote: > On Wed, Nov 2, 2022 at 7:12 PM Palmer Dabbelt wrote: >> >> On Wed, 02 Nov 2022 10:19:15 PDT (-0700), gcc-patches@gcc.gnu.org wrote: >> > Could you add some test cases? >> >> Also documentation, and ideally some sort of spec for what this should >> do so we can maintain compatibility with LLVM as well as we can. >> >> IIUC this also allows for profiles in the arch function attributes, >> which would end up plumbing through to the assembler so we'd need >> support there? Probably best to just expand these out for the rest of >> the tools so we don't need the profile->extension mappings everywhere, >> IMO it's the same as the -mcpu discussion. >> >> > >> > --- >> > >> > Parsing logic is kind of too adhoc, I would prefer using something >> > like the following code to prevent magic pointer arithmetic like p+6: >> > >> > something like this: >> > >> > Table of all profile names = {"RVA20U64", riscv_profile::RVA20U64, ...} >> > >> > const char *rva20u64[] = {"m", "a", "f", "d",... NULL}; >> > >> > table of profile content = >> > { >> > {riscv_profile::RVA20U64, rva20u64}, >> > .. >> > } >> > >> > parse march () >> > { >> > if march is startswith >> > else if ((profile = parse_proile(march)) != risv_profile::NOT_PROFILE) >> > handle_profile (profile) >> > else >> > error >> > } >> > >> > handle_profile (profile) >> > { >> > use table of profile content to update ext. >> > } >> > >> > >> > On Wed, Nov 2, 2022 at 5:54 AM jiawei wrote: >> >>handle_profile >> >> Add two new function to handle profile input, >> >> "parse_profile" will check if a input into -march is >> >> legal, if it is then "handle_profile" will check the >> >> profile's type[I/M/A], year[20/22] and mode[U/S/M], >> >> set different extensions combine, just deal mandatory >> >> part currently. >> >> >> >> gcc/ChangeLog: >> >> >> >> * common/config/riscv/riscv-common.cc >> >> (riscv_subset_list::parse_profile): Check if profile name is valid or not. >> >> (riscv_subset_list::parse_std_ext): If input of -march option is >> >> a profile,skip first ISA check. >> >> (riscv_subset_list::parse): Handle rofile input in -march. >> >> (riscv_subset_list::handle_profile): Handle differen profiles >> >> expand to extensions. >> >> * config/riscv/riscv-subset.h: New function prototypes. >> >> >> >> >> >> --- >> >> gcc/common/config/riscv/riscv-common.cc | 95 +++++++++++++++++++++++-- >> >> gcc/config/riscv/riscv-subset.h | 5 +- >> >> 2 files changed, 94 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc >> >> index 602491c638d..da06bd89144 100644 >> >> --- a/gcc/common/config/riscv/riscv-common.cc >> >> +++ b/gcc/common/config/riscv/riscv-common.cc >> >> @@ -777,6 +777,35 @@ riscv_subset_list::parsing_subset_version (const char *ext, >> >> return p; >> >> } >> >> >> >> +/* Parsing function for profile. >> >> + >> >> + Return Value: >> >> + Points to the end of profile. >> >> + >> >> + Arguments: >> >> + `p`: Current parsing position. */ >> >> + >> >> +const char * >> >> +riscv_subset_list::parse_profile (const char *p) >> >> +{ >> >> + if(*p == 'I' || *p == 'M' || *p == 'A'){ >> >> + p++; >> >> + if(startswith (p, "20") || startswith (p, "22")) >> >> + p += 2; >> >> + if (*p == 'U' || *p == 'S' || *p == 'M') >> >> + p++; >> >> + if(startswith (p, "64") || startswith (p, "32")){ >> >> + p += 2; >> >> + riscv_subset_list::handle_profile(p-6, p-4, p-3); >> >> + return p; >> >> + } >> >> + } >> >> + else >> >> + error_at (m_loc, "%<-march=%s%>: Invalid profile.", m_arch); >> >> + return NULL; >> >> +} >> >> + >> >> + >> >> /* Parsing function for standard extensions.parse_std_ext >> >> >> > >> > It's sort of too adhoc parsing the profile name, I would prefer using >> > something like the following code to prevent magic pointer arithmetic >> > like p+6. >> > something >> > Table of all profile names = {"RVA20U64", riscv_profile::RVA20U64, ...} >> > >> > const char *rva20u64[] = {"m", "a", "f", "d",... NULL}; >> > >> > table of profile content = >> > { >> > {riscv_profile::RVA20U64, rva20u64}, >> > .. >> > } >> > >> > parse march () >> > { >> > if march is startswith >> > else if ((profile = parse_proile(march)) != risv_profile::NOT_PROFILE) >> > handle_profile (profile) >> > else >> > error >> > } >> > >> > handle_profile (profile) >> > { >> > ad >> > } >> > >> >> Return Value: >> >> @@ -786,7 +815,7 @@ riscv_subset_list::parsing_subset_version (const char *ext, >> >> `p`: Current parsing position. */ >> >> >> >> const char * >> >> -riscv_subset_list::parse_std_ext (const char *p) >> >> +riscv_subset_list::parse_std_ext (const char *p, bool isprofile) >> >> { >> >> const char *all_std_exts = riscv_supported_std_ext (); >> >> const char *std_exts = all_std_exts; >> >> @@ -795,8 +824,8 @@ riscv_subset_list::parse_std_ext (const char *p) >> >> unsigned minor_version = 0; >> >> char std_ext = '\0'; >> >> bool explicit_version_p = false; >> >> - >> >> - /* First letter must start with i, e or g. */ >> >> + if (!isprofile){ >> >> + /* First letter must start with i, e or g. */ >> >> switch (*p) >> >> { >> >> case 'i': >> >> @@ -850,6 +879,7 @@ riscv_subset_list::parse_std_ext (const char *p) >> >> "% or %", m_arch); >> >> return NULL; >> >> } >> >> +} >> >> >> >> while (p != NULL && *p) >> >> { >> >> @@ -1093,6 +1123,7 @@ riscv_subset_list::parse (const char *arch, location_t loc) >> >> riscv_subset_list *subset_list = new riscv_subset_list (arch, loc); >> >> riscv_subset_t *itr; >> >> const char *p = arch; >> >> + bool isprofile = false; >> >> if (startswith (p, "rv32")) >> >> { >> >> subset_list->m_xlen = 32; >> >> @@ -1103,15 +1134,26 @@ riscv_subset_list::parse (const char *arch, location_t loc) >> >> subset_list->m_xlen = 64; >> >> p += 4; >> >> } >> >> + else if (startswith (p, "RV")) >> >> + { >> >> + if (startswith (p+6, "64")) >> >> + subset_list->m_xlen = 64; >> >> + else >> >> + subset_list->m_xlen = 32; >> >> + p += 2; >> >> + /* Parsing profile name. */ >> >> + p = subset_list->parse_profile (p); >> >> + isprofile = true;handle_profile >> >> + } >> >> else >> >> { >> >> - error_at (loc, "%<-march=%s%>: ISA string must begin with rv32 or rv64", >> >> + error_at (loc, "%<-march=%s%>: ISA string must begin with rv32 , rv64 or a profile", >> >> arch); >> >> IMO we really don't want profiles in -march. Unless I'm missing a >> recent change, the profiles are a different namespace from the base ISAs >> and trying to mix them into the same namespace is just going to lead to >> headaches in the future. We've got enough complexity with ISA strings >> as it is. > > Thanks for pointing this out! A potential conflicting namespace is > indeed a clear sign to not merge the ISA string and the profiles. > A change to clarify the compatibility of the ISA string and the > profiles has been suggested to the profiles spec and got accepted and > merged with the words "Yes, this is guaranteed": > https://github.com/riscv/riscv-profiles/pull/78 > > With this change in the profiles spec, we have the guarantee that the > namespaces can be merged and thus there should be no namespace issue > anymore with specifying the profile via -march. As per the discussion on the RISC-V lists, that's not quite what we're looking for. Sounds like the desired behavior here is actually to just not have -march take ISA strings. That's fine with me, I just sent a patch to change the docs. It's a pretty big policy change, though, so let's have the discussion over there to make sure it's more visible? > >> >> >> goto fail; >> >> } >> >> >> >> /* Parsing standard extension. */ >> >> - p = subset_list->parse_std_ext (p); >> >> + p = subset_list->parse_std_ext (p,isprofile); >> >> >> >> if (p == NULL) >> >> goto fail; >> >> @@ -1349,6 +1391,49 @@ riscv_handle_option (struct gcc_options *opts, >> >> } >> >> } >> >> >> >> +/* Expand profile with defined mandatory extensions, >> >> + M-type/mode is emtpy and set as base right now. */ >> >> +void riscv_subset_list::handle_profile(const char *profile_type, >> >> + const char *profile_year, >> >> + const char *profile_mode) >> >> +{ >> >> + add ("i", false); >> >> + if(*profile_type == 'A'){ >> >> + add ("m", false); >> >> + add ("a", false); >> >> + add ("f", false); >> >> + add ("d", false); >> >> + add ("c", false); >> >> + add ("ziccamoa", false); >> >> + add ("ziccif", false); >> >> + add ("zicclsm", false); >> >> + add ("ziccrse", false); >> >> + add ("zicntr", false); >> >> + add ("zicsr", false); >> >> + >> >> + if(*profile_mode == 'S') >> >> + add ("zifencei", false); >> >> Various other bits of the ISA specs say we're meant to ignore the >> supervisor bits from compilers. I'm not sure if that's true for the >> profiles too? >> >> Though Zifencei is a doubly-complicated case, as it's forbidden by the >> Linux uABI... >> >> >> + >> >> + if(*profile_year == '2') >> >> I think Kito pointed that out before, but the parsing here is pretty >> ad-hoc. This one looks buggy, though: it's treating all 2* profiles the >> same, despite 20 being different from 22 and things like 21 and 23 being >> undefined. >> >> >> + { >> >> + add ("zihintpause", false); >> >> + add ("zihpm", false); >> >> + add ("zba", false); >> >> + add ("zbb", false); >> >> + add ("zbs", false); >> >> + add ("zicbom", false); >> >> + add ("zicbop", false); >> >> + add ("zicboz", false); >> >> + add ("zfhmin", false); >> >> + add ("zkt", false); >> >> + if(*profile_mode == 'S'){ >> >> + add ("svpbmt", false); >> >> + add ("svinval", false); >> >> + } >> >> + } >> >> + } >> >> +} >> >> Looks like there's nothing here that handles the optional, unsupported, >> and unmentioned extensions? It's not super clear what we should do with >> those, but whatever it is we should document it so users aren't >> surprised. >> >> This also doesn't cover the ISA spec versioning, which as far as I can >> tell profiles don't directly mention but implicitly depend on (things >> like having a mandatory fence.tso, for example). >> >> >> + >> >> /* Expand arch string with implied extensions. */ >> >> >> >> const char * >> >> diff --git a/gcc/config/riscv/riscv-subset.h b/gcc/config/riscv/riscv-subset.h >> >> index 0bb3a9d29d0..303be5ed9ed 100644 >> >> --- a/gcc/config/riscv/riscv-subset.h >> >> +++ b/gcc/config/riscv/riscv-subset.h >> >> @@ -62,13 +62,16 @@ private: >> >> const char *parsing_subset_version (const char *, const char *, unsigned *, >> >> unsigned *, bool, bool *); >> >> >> >> - const char *parse_std_ext (const char *); >> >> + const char *parse_profile (const char *); >> >> + >> >> + const char *parse_std_ext (const char *, bool); >> >> >> >> const char *parse_multiletter_ext (const char *, const char *, >> >> const char *); >> >> >> >> void handle_implied_ext (riscv_subset_t *); >> >> void handle_combine_ext (); >> >> + void handle_profile(const char *, const char *, const char *); >> >> >> >> public: >> >> ~riscv_subset_list (); >> >> -- >> >> 2.25.1 >> >>