From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk1-xa2a.google.com (mail-vk1-xa2a.google.com [IPv6:2607:f8b0:4864:20::a2a]) by sourceware.org (Postfix) with ESMTPS id DE36E3858C30 for ; Sat, 23 Sep 2023 09:04:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DE36E3858C30 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-vk1-xa2a.google.com with SMTP id 71dfb90a1353d-493a661d7b6so3304124e0c.1 for ; Sat, 23 Sep 2023 02:04:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695459883; x=1696064683; darn=gcc.gnu.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=L10o7nscvoh0qxNqS8A49SNLD3GN+d4zBdxVZgPODBY=; b=f9seh9epUUBOe44UnVocUGSKDLr6tPAnKDiPlp/jDvUZRes8h2T7n87c8+sz0MLDkp mTen0FNJzVBblFjMPti5wPHfkgX71Y8VWtaMH1z3ZUiH/hoBdFaCcpy5dtF+8gwGjYYd T8thXSHOvDCMKycIpjnoCRBczwNte2BPTA0L5d0OuifR1/qEXHnQmCCMskkKYR0BO8gY +szpoz8xLpJJKMZd0lmYvaxGzsyZFLIyVmRd5iGAd5KGYx3+hvoY9b1HGscg7WLLAqje X8mQbYdOXsYxhjgfGETtJ6qVxDe4ku2Ye7WTh93MeW5jA6eVkMB2qOFa3MXGCM6Z+bk+ Sumg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695459883; x=1696064683; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=L10o7nscvoh0qxNqS8A49SNLD3GN+d4zBdxVZgPODBY=; b=mXs2RzTqXijG/0Lh8hurjzNnZj9pDoR3lAzZliW/M2ANiH2KQVNcMJo3uUxT1kqHKv zhDxJ4X2MRiR/nluu5BGEVvGMk3xYRIm55pY0ByBuyTqVu1162RpU/yI2CPTJYUY01bs Ma72yDqNUrLiHg9z2mIO70cNQy2tdszxXxMvxckSFsXThlrSeFcTiPUmqV1MRAmodTCg 9dSVlkR03jINBx1Eg8enkYjfuFC4E150+MD5dF18t8mzoXmnMPLZNjdeLTuFC0y4PK9l c8tJ3l04gMb97XwXTM0jIerzpw3s+U8c+Gxkbg63//nzwsSklZ2Zlw5bkuoyBVpeu9Pp pksw== X-Gm-Message-State: AOJu0YwOsX+4V+4wf8UOOMEe6yqO2ONGp9t8fFx69GAlPNgRhydgPJYS RSCop0+Y0yM3Sdj/E2kbnaxz2Ez93FzXON96HvdxcKmEtQEimcmi X-Google-Smtp-Source: AGHT+IGKATiMdmnZgqK6alsE4TfpPXHpK8JjjsPUBgabwnIr1wEsKrz6x0+k+mJiST2SBMMsDybLB9DBlr8vlICot9k= X-Received: by 2002:a67:fe45:0:b0:452:6ecb:e90 with SMTP id m5-20020a67fe45000000b004526ecb0e90mr954130vsr.3.1695459883037; Sat, 23 Sep 2023 02:04:43 -0700 (PDT) MIME-Version: 1.0 References: <20230919150734.2854664-1-mary.bennett@embecosm.com> <20230919150734.2854664-3-mary.bennett@embecosm.com> In-Reply-To: <20230919150734.2854664-3-mary.bennett@embecosm.com> From: Kito Cheng Date: Sat, 23 Sep 2023 10:04:32 +0100 Message-ID: Subject: Re: [PATCH 2/2] RISC-V: Add support for XCValu extension in CV32E40P To: Mary Bennett Cc: gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,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: Hi Mary: Several inline comments, mostly are related to the RTX pattern. I guess we don't really need those unspec except clip*. > diff --git a/gcc/config/riscv/corev.md b/gcc/config/riscv/corev.md > index 59aeafe485f..30c8bcbe476 100644 > --- a/gcc/config/riscv/corev.md > +++ b/gcc/config/riscv/corev.md > @@ -17,6 +17,27 @@ > ;; along with GCC; see the file COPYING3. If not see > ;; . > > +(define_c_enum "unspec" [ > + > + ;;CORE-V ALU > + UNSPEC_CV_ALU_CLIP > + UNSPEC_CV_ALU_CLIPR > + UNSPEC_CV_ALU_CLIPU > + UNSPEC_CV_ALU_CLIPUR > + UNSPEC_CV_ALU_ADDN > + UNSPEC_CV_ALU_ADDUN > + UNSPEC_CV_ALU_ADDRN > + UNSPEC_CV_ALU_ADDURN > + UNSPEC_CV_ALU_SUBN > + UNSPEC_CV_ALU_SUBUN > + UNSPEC_CV_ALU_SUBRN > + UNSPEC_CV_ALU_SUBURN > + UNSPEC_CV_ALU_EXTHS > + UNSPEC_CV_ALU_EXTHZ > + UNSPEC_CV_ALU_EXTBS > + UNSPEC_CV_ALU_EXTBZ > +]) > + > ;; XCVMAC extension. > > (define_insn "riscv_cv_mac_mac" > @@ -388,3 +409,267 @@ > "cv.machhsRN\t%0,%1,%2,%4" > [(set_attr "type" "arith") > (set_attr "mode" "SI")]) > + > +;; XCVALU builtins > + > +(define_insn "riscv_cv_alu_slet" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (if_then_else > + (gt:SI > + (match_operand:SI 1 "register_operand" "r") > + (match_operand:SI 2 "register_operand" "r")) > + (const_int 0) > + (const_int 1)))] Maybe just something like that? slt instruction using similar pattern like that. (set (match_operand:SI 0 "register_operand" "=r") (gt:SI (match_operand:SI 1 "register_operand" "r") (match_operand:SI 2 "register_operand" "r"))) > + > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.slet\t%0, %1, %2" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_sletu" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (if_then_else > + (gtu:SI > + (match_operand:SI 1 "register_operand" "r") > + (match_operand:SI 2 "register_operand" "r")) > + (const_int 0) > + (const_int 1)))] Same comment as riscv_cv_alu_slet. > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.sletu\t%0, %1, %2" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_min" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (if_then_else > + (gt:SI > + (match_operand:SI 1 "register_operand" "r") > + (match_operand:SI 2 "register_operand" "r")) > + (match_dup:SI 2) > + (match_dup:SI 1)))] Maybe something like that? (set (match_operand:SI 0 "register_operand" "=r") (min (match_operand:SI 1 "register_operand" "r") (match_operand:SI 2 "register_operand" "r"))) > + > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.min\t%0, %1, %2" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_minu" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (if_then_else > + (gtu:SI > + (match_operand:SI 1 "register_operand" "r") > + (match_operand:SI 2 "register_operand" "r")) > + (match_dup:SI 2) > + (match_dup:SI 1)))] (set (match_operand:SI 0 "register_operand" "=r") (minu (match_operand:SI 1 "register_operand" "r") (match_operand:SI 2 "register_operand" "r"))) > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.minu\t%0, %1, %2" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_max" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (if_then_else > + (gt:SI > + (match_operand:SI 1 "register_operand" "r") > + (match_operand:SI 2 "register_operand" "r")) > + (match_dup:SI 1) > + (match_dup:SI 2)))] (set (match_operand:SI 0 "register_operand" "=r") (max (match_operand:SI 1 "register_operand" "r") (match_operand:SI 2 "register_operand" "r"))) > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.max\t%0, %1, %2" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_maxu" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (if_then_else > + (gtu:SI > + (match_operand:SI 1 "register_operand" "r") > + (match_operand:SI 2 "register_operand" "r")) (set (match_operand:SI 0 "register_operand" "=r") (maxu (match_operand:SI 1 "register_operand" "r") (match_operand:SI 2 "register_operand" "r"))) > + (match_dup:SI 1) > + (match_dup:SI 2)))] > + > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.maxu\t%0, %1, %2" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_exths" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (unspec:SI [(match_operand:HI 1 "register_operand" "r")] > + UNSPEC_CV_ALU_EXTHS))] It's sign-extension from HI to SI? if so that should just using sign_extend rather than unspec [(set (match_operand:SI 0 "register_operand" "=r") (sign_extend:SI (match_operand:HI 1 "nonimmediate_operand" "r")))] > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.exths\t%0, %1" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_exthz" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (unspec:SI [(match_operand:HI 1 "register_operand" "r")] > + UNSPEC_CV_ALU_EXTHZ))] same comment as riscv_cv_alu_exths but zero_extend > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.exthz\t%0, %1" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_extbs" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (unspec:SI [(match_operand:QI 1 "register_operand" "r")] > + UNSPEC_CV_ALU_EXTBS))] > + same comment as riscv_cv_alu_exths with QI > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.extbs\t%0, %1" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_extbz" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (unspec:SI [(match_operand:QI 1 "register_operand" "r")] > + UNSPEC_CV_ALU_EXTBZ))] same comment as riscv_cv_alu_exths with QI and zero_extend > + "TARGET_XCVALU && !TARGET_64BIT" > + "cv.extbz\t%0, %1" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_clip" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,r") > + (match_operand:SI 2 "immediate_register_operand" "CVP2,r")] > + UNSPEC_CV_ALU_CLIP))] > + > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.clip\t%0,%1,%X2 > + cv.clipr\t%0,%1,%2" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_clipu" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,r") > + (match_operand:SI 2 "immediate_register_operand" "CVP2,r")] > + UNSPEC_CV_ALU_CLIPU))] > + > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.clipu\t%0,%1,%X2 > + cv.clipur\t%0,%1,%2" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_addN" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,0") > + (match_operand:SI 2 "register_operand" "r,r") > + (match_operand:QI 3 "csr_operand" "K,r")] > + UNSPEC_CV_ALU_ADDN))] I think this should be able to using generic RTL rather than unspec to represent that? e.g. (set (match_operand) (ashiftrt (plus (match_operand) (match_operand)) (match_operand))) > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.addN\t%0,%1,%2,%3 > + cv.addNr\t%0,%2,%3" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_adduN" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,0") > + (match_operand:SI 2 "register_operand" "r,r") > + (match_operand:QI 3 "csr_operand" "K,r")] > + UNSPEC_CV_ALU_ADDUN))] > + same comment as riscv_cv_alu_addN but lshiftrt > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.adduN\t%0,%1,%2,%3 > + cv.adduNr\t%0,%2,%3" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_addRN" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,0") > + (match_operand:SI 2 "register_operand" "r,r") > + (match_operand:QI 3 "csr_operand" "K,r")] > + UNSPEC_CV_ALU_ADDRN))] same comment as riscv_cv_alu_addN but few more complicated pattern. > + > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.addRN\t%0,%1,%2,%3 > + cv.addRNr\t%0,%2,%3" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_adduRN" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,0") > + (match_operand:SI 2 "register_operand" "r,r") > + (match_operand:QI 3 "csr_operand" "K,r")] > + UNSPEC_CV_ALU_ADDURN))] same comment as riscv_cv_alu_addN but few more complicated pattern. > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.adduRN\t%0,%1,%2,%3 > + cv.adduRNr\t%0,%2,%3" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_subN" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,0") > + (match_operand:SI 2 "register_operand" "r,r") > + (match_operand:QI 3 "csr_operand" "K,r")] > + UNSPEC_CV_ALU_SUBN))] same comment as riscv_cv_alu_addN but few more complicated pattern. > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.subN\t%0,%1,%2,%3 > + cv.subNr\t%0,%2,%3" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_subuN" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,0") > + (match_operand:SI 2 "register_operand" "r,r") > + (match_operand:QI 3 "csr_operand" "K,r")] > + UNSPEC_CV_ALU_SUBUN))] same comment as riscv_cv_alu_addN but few more complicated pattern. > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.subuN\t%0,%1,%2,%3 > + cv.subuNr\t%0,%2,%3" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_subRN" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,0") > + (match_operand:SI 2 "register_operand" "r,r") > + (match_operand:QI 3 "csr_operand" "K,r")] > + UNSPEC_CV_ALU_SUBRN))] same comment as riscv_cv_alu_addN but few more complicated pattern. > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.subRN\t%0,%1,%2,%3 > + cv.subRNr\t%0,%2,%3" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")]) > + > +(define_insn "riscv_cv_alu_subuRN" > + [(set (match_operand:SI 0 "register_operand" "=r,r") > + (unspec:SI [(match_operand:SI 1 "register_operand" "r,0") > + (match_operand:SI 2 "register_operand" "r,r") > + (match_operand:QI 3 "csr_operand" "K,r")] > + UNSPEC_CV_ALU_SUBURN))] same comment as riscv_cv_alu_addN but few more complicated pattern. > + "TARGET_XCVALU && !TARGET_64BIT" > + "@ > + cv.subuRN\t%0,%1,%2,%3 > + cv.subuRNr\t%0,%2,%3" > + [(set_attr "type" "arith") > + (set_attr "mode" "SI")])