From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id CH8pJyYCfWC4ZQAAWB0awg (envelope-from ) for ; Mon, 19 Apr 2021 00:08:06 -0400 Received: by simark.ca (Postfix, from userid 112) id 8DA201F104; Mon, 19 Apr 2021 00:08:06 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id F15061E813 for ; Mon, 19 Apr 2021 00:08:04 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 839103952022; Mon, 19 Apr 2021 04:08:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 839103952022 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1618805284; bh=gXgccDXUK75U6mqpsTBBgEtYENmgpmBE/o8W4Xi2GMc=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=wDVGfVBI5VHV6o+lo/0Mn+3Sq+ezVnH4LyMIbGBmfVruHz6Id6hqpQQB54b/i9yor wDFw3KyG4xiFF6IezmdDN/anc8IL6PpoZtu6q2eiTPy1wOmZoIdMxUqeypN9Nv9sMw YGwqwG/pwozIhYX+jGwys8xrmh1DeOGJPnQDz//Y= Received: from smtp.gentoo.org (woodpecker.gentoo.org [IPv6:2001:470:ea4a:1:5054:ff:fec7:86e4]) by sourceware.org (Postfix) with ESMTP id EDD043851C22 for ; Mon, 19 Apr 2021 04:08:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EDD043851C22 Received: from vapier (localhost [127.0.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.gentoo.org (Postfix) with ESMTPS id 10FD9340E13; Mon, 19 Apr 2021 04:08:00 +0000 (UTC) Date: Mon, 19 Apr 2021 00:08:00 -0400 To: Jim Wilson Subject: Re: [PATCH 06/24] RISC-V: Add fp support. Message-ID: Mail-Followup-To: Jim Wilson , gdb-patches@sourceware.org, Monk Chiang References: <20210417175831.16413-1-jimw@sifive.com> <20210417175831.16413-7-jimw@sifive.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210417175831.16413-7-jimw@sifive.com> X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mike Frysinger via Gdb-patches Reply-To: Mike Frysinger Cc: Monk Chiang , gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 17 Apr 2021 10:58, Jim Wilson wrote: > Add F and D instruction support. substance looks fine, just style nits tests ? > +execute_d (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > +{ > ... > + static const int round_modes[] = > + { > + sim_fpu_round_near, sim_fpu_round_zero, > + sim_fpu_round_down, sim_fpu_round_up, > + sim_fpu_round_default, sim_fpu_round_default, > + sim_fpu_round_default > + }; hanging indent should be 2 spaces, not 4 > + u32 |= (cpu->fpregs[rs1].w[1] & 0x80000000) ^ (cpu->fpregs[rs2].w[1] & 0x80000000); line is too long -- wrap to 80 cols. i didn't check every line ... this one just stood out. so please give it a double check. > + switch (sim_fpu_is (&sfa)) > + { > + case SIM_FPU_IS_NINF: > + cpu->regs[rd] = 1; > + break; > + case SIM_FPU_IS_NNUMBER: > + cpu->regs[rd] = 1 << 1; i'm a little surprised all these bits don't have constants for them. but i guess binutils wouldn't normally have broken out the FPU state into headers ? > +static sim_cia > +execute_f (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > +{ > ... > + static const int round_modes[] = > + { > + sim_fpu_round_near, sim_fpu_round_zero, > + sim_fpu_round_down, sim_fpu_round_up, > + sim_fpu_round_default, sim_fpu_round_default, > + sim_fpu_round_default > + }; use 2 space hanging indent > + case MATCH_FCVT_L_S: > + TRACE_INSN (cpu, "fcvt.l.s %s, %s", > + rd_name, frs1_name); > + cpu->regs[rd] = (int64_t) cpu->fpregs[rs1].S[0]; > + goto done; > + case MATCH_FCVT_LU_S: > + TRACE_INSN (cpu, "fcvt.lu.s %s, %s", > + rd_name, frs1_name); > + cpu->regs[rd] = (uint64_t) cpu->fpregs[rs1].S[0]; > + goto done; > + case MATCH_FCVT_S_L: > + TRACE_INSN (cpu, "fcvt.s.l %s, %s", > + frd_name, rs1_name); > + cpu->fpregs[rd].S[0] = (float) ((int64_t) cpu->regs[rs1]); > + goto done; > + case MATCH_FCVT_S_LU: > + TRACE_INSN (cpu, "fcvt.s.lu %s, %s", > + frd_name, rs1_name); > + cpu->fpregs[rd].S[0] = (float) cpu->regs[rs1]; these raw casts all feel ... wrong. are the semantics guaranteed to match between whatever the host CPU is (e.g. x86_64) and the target (e.g. riscv) ? or it just seems to mostly work so we aren't going to squint too hard at it ? > --- a/sim/riscv/sim-main.h > +++ b/sim/riscv/sim-main.h > > +typedef union FRegisterValue > +{ > + uint64_t v[2]; > + uint32_t w[4]; > + > + int64_t V[2]; > + int32_t W[4]; > + > + float S[4]; > + double D[2]; > + > +} FRegister; trim that trailing blank line -mike