From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1031.google.com (mail-pj1-x1031.google.com [IPv6:2607:f8b0:4864:20::1031]) by sourceware.org (Postfix) with ESMTPS id 950AB3857402 for ; Wed, 2 Nov 2022 18:11:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 950AB3857402 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-x1031.google.com with SMTP id u8-20020a17090a5e4800b002106dcdd4a0so2892336pji.1 for ; Wed, 02 Nov 2022 11:11:46 -0700 (PDT) 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=PzcrMEu/Eep/tCcgMBWFizVRlWOG41zNoU2pfRsXQns=; b=ttDdlo9g6cemNMitskVP8Y9DV2jKLrF/bk/Hw6o8dwyf+RVd1OHc79/XxkIDJoK0fQ iwD+d1kTqe4iSbEAcnUY3DbDKA5SXxCEEuEuu4H4vjldxxUSfe1lD1Q7JaynIoine1uO XdxPcjksPpn8MhCbbl+/9Z8FusQmakjsnDFy/fYuh2n40zSHLJ81z8maYPGBGQdnP1RK nb37/Sel/dWkB+1cc6XpRCjCtfyOgnqjgv6ISFG+Eih4jDMkaQq9wOz5SchycH7Y6OJ6 nMw/A2sC5pDg++0vakSIS4HN1QdptkQh9yohZtVUe5GHiGV9rIdjme/VNE56aPnIA5Gi iDVg== 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=PzcrMEu/Eep/tCcgMBWFizVRlWOG41zNoU2pfRsXQns=; b=AzNWWQI8uCF3iUiKHouKbZaKX83yuXDsCo9cWLSKxaF8HscXOXIEIOAa3WYjkFyue5 2JYUOPoD0TJM2csV1tRq7LqluYrNK2+dZgQdEGMFJUNz77CQhqK/kBKGQnoLUFuPzher y8lpNxGVXGw7xvU7abzqO2bBT+Hr4/mvGpvTIExMWHD2+0pgE2Tb7enlTDmfB8UsnpFb QXcRS2+Kco/4lYXj8SKlvx6nkoKk8zmninmD9yr1mMQgNO6FvXb/CgMQNy/exMfhpl9W 4k5ZzLXGuBH6uixIbFUBX5ZBxxSPi474E0qx2741xrCkAfP/3ngNHDq55hTK094d0vEV qmyA== X-Gm-Message-State: ACrzQf2yZB/Ws+TCrX51Dk4t0IaPtJExS3VFsnH2UN2pWlZMMaGMSd1k 25nAQV5zwT6YKsG8bhDcobiQxw== X-Google-Smtp-Source: AMsMyM4U1Jp0H55gE9gQsHAm/RKapvPGf7FV+mgTRqXo9TVDrevZyNy0CtY5iopIBi8+LU0YUsgmzQ== X-Received: by 2002:a17:90a:df04:b0:213:1cf1:435d with SMTP id gp4-20020a17090adf0400b002131cf1435dmr27670358pjb.210.1667412705139; Wed, 02 Nov 2022 11:11:45 -0700 (PDT) Received: from localhost ([50.221.140.188]) by smtp.gmail.com with ESMTPSA id i2-20020a170902cf0200b00183ba0fd54dsm8646108plg.262.2022.11.02.11.11.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 02 Nov 2022 11:11:44 -0700 (PDT) Date: Wed, 02 Nov 2022 11:11:44 -0700 (PDT) X-Google-Original-Date: Wed, 02 Nov 2022 11:11:45 PDT (-0700) Subject: Re: [RFC] RISC-V: Add profile supports. In-Reply-To: CC: jiawei@iscas.ac.cn, kito.cheng@sifive.com, wuwei2016@iscas.ac.cn, gcc-patches@gcc.gnu.org, philipp.tomsich@vrull.eu From: Palmer Dabbelt To: gcc-patches@gcc.gnu.org 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.7 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 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. >> 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 >>