From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B163E81FEE for ; Mon, 5 Dec 2016 01:35:37 -0800 (PST) Received: by mail-wm0-x233.google.com with SMTP id g23so87743103wme.1 for ; Mon, 05 Dec 2016 01:35:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=7uQ6uMgXAkhD0DhQaTJVI1w0Ql62iPJzjNwm7iwvWhI=; b=O8Axs8wMeXf+eoyeo+camZhV8J1SHu9uwBAiF95JNWgRAOsRXjamUgw1DpK1f/2cPO bH4pdfNXtKy484D/aLiZbU260F4SB57KQbJLiiHjV4ajoVgh/ZhvT4TAjj+IZKXXpgzA /7WkxA9yWacAgxMusiEB3cpWWylcWmQ6RUZ74= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=7uQ6uMgXAkhD0DhQaTJVI1w0Ql62iPJzjNwm7iwvWhI=; b=kGE1cCHwKmUA3hj/4KQWWTmpSe7Ojj8BK/bieEdsCgdwKUaGl5Zb0JWCL1FZUNOlDT Bnb/+yur8tCTsiWnaYzx3YV9YmpMznp1jvVSTQ0p6+54q+zHu75atGaSYr05E3ATw5l3 pseXnKkyV7idcbrHojZ8x7NlnNYNKmWbqJaRLRevu+nNwUbXRZzZuP2UirLiJqeTc+2q adkP7UysSA6wMcYcEJp11m0fVDsWzUPq/LVMtJEHJvXMN/JdSnNe2BaNzEck1MHoqQUT lCNtkW88ehqLNUzNMer7v3bcbb1qlyImaeGaP0zzs0sWfWtACzke6cZu9DxAV2XjHDB5 K8Jw== X-Gm-Message-State: AKaTC03Xy1VR6AZdR9RdNFmggWPuSNdP1JUU4fTewr+ZuR2wAeSp1WuyjQJdmXJP18rJA3L4 X-Received: by 10.28.151.136 with SMTP id z130mr8969784wmd.126.1480930535898; Mon, 05 Dec 2016 01:35:35 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id c81sm17003148wmf.22.2016.12.05.01.35.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Dec 2016 01:35:35 -0800 (PST) Date: Mon, 5 Dec 2016 09:35:33 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: "Ni, Ruiyu" , "Tian, Feng" , "edk2-devel@lists.01.org" , "afish@apple.com" , "Gao, Liming" , "Kinney, Michael D" Message-ID: <20161205093533.GE27069@bivouac.eciton.net> References: <1480088538-21834-1-git-send-email-ard.biesheuvel@linaro.org> <1480088538-21834-4-git-send-email-ard.biesheuvel@linaro.org> <734D49CCEBEEF84792F5B80ED585239D58E8CFA1@SHSMSX104.ccr.corp.intel.com> <20161130140655.GG27069@bivouac.eciton.net> <734D49CCEBEEF84792F5B80ED585239D58E926A5@SHSMSX104.ccr.corp.intel.com> <20161202144050.GV27069@bivouac.eciton.net> <20161202155637.GB27069@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Dec 2016 09:35:38 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Dec 02, 2016 at 06:42:58PM +0000, Ard Biesheuvel wrote: > >> > The point is that there is nothing clever here for the compiler to do: > >> > casting a 64-bit int to a 32-bit int is an operation that discards the > >> > top 32 bits - but the behaviour is entirely defined. > >> > > >> > In fact, all architectures other than IA32/ARM use the Math64.c > >> > implementation of RShiftU64, which simply does the shift. > >> > And ARM does it only for GCC. I'm not sure why, I can't imagine a GCC > >> > version that would do anything more complicated than > >> > --- > >> > 00000000 : > >> > 0: e1a000c1 asr r0, r1, #1 > >> > 4: e12fff1e bx lr > >> > --- > >> > (and that's with -march=armv4t onwards) > >> > >> That's a 32-bit operand. Shifting a 64-bit value will result in a > >> call to a compiler intrinsic, which is kind of the point of Riuyu's > >> remark > > > > No, it's not. > > As stated, that is the output from compiling: > > > > unsigned int shift(long long val) > > { > > return (val >> 33); > > } > > > > Unless you are claiming long long is 32-bit on ARM. > > > > Although I did mess up and make it signed :) > > When fixed, the code generation is identical but with an lsr instead. > > > > The compiler just operates directly on the top 32-bits (in r1), since > > the ones in r0 are irrelevant to the result. > > OK, but that does mean it is performing optimization up to some point. > So right shifts of 64-bit quantities by 32 bits or more will be > 'optimized' by GCC into something that does not rely on the > intrinsics. But this is not universally true for all toolchains And I'm OK with a statement of "we can't do this because version X of toolchain Y does not support it". But I am much less happy for people to introduce workarounds in functional code because of an issue that may no longer be relevant for any toolchains actually in use (or even supported by BaseTools). At which point can we know this problem goes away and we can program in regular C instead, if we decide to obsolete some extremely outdated toolchains? > (and variable shifts are probably treated differently as well) It has no effect on intrinsics being required or not, it simply requires some more stack shuffling. But yes, that removes all options for compiler shortcuts. > > Change the shift to 31 and the compiler doesn't do anything cute, so > > the code is slightly more obvious: > > --- > > 00000000 : > > 0: e1a00fa0 lsr r0, r0, #31 > > 4: e1800081 orr r0, r0, r1, lsl #1 > > 8: e12fff1e bx lr > > --- > > > > But even if there were intrinsics generated, shouldn't we support the > > intrinsic instead of massaging the C code and hope we can prevent it > > from being generated. > > This is exactly the issue at hand: EDK2 typically does *not* rely on > such intrinsics, even if we were forced to add them for ARM because > GCC cannot be instructed to inhibit such calls. > > Actually, I do not agree with the EDK2 position, i.e., I think there > is no issue relying on routines that are typically provided by the C > runtime. But this is MdeModulePkg, so it is not up to us. Indeed it is not. But this does not sound like a topic we want per-package rules on. Regards, Leif