From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9022 invoked by alias); 6 Sep 2019 13:01:21 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 9014 invoked by uid 89); 6 Sep 2019 13:01:21 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-4.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.1 spammy=H*i:sk:CAFiYyc, naturally, optimizers X-HELO: mx0b-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0b-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Sep 2019 13:01:19 +0000 Received: from pps.filterd (m0127361.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x86Cwau7076278 for ; Fri, 6 Sep 2019 09:01:17 -0400 Received: from e06smtp04.uk.ibm.com (e06smtp04.uk.ibm.com [195.75.94.100]) by mx0a-001b2d01.pphosted.com with ESMTP id 2uunkscktj-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 06 Sep 2019 09:01:15 -0400 Received: from localhost by e06smtp04.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 6 Sep 2019 14:00:11 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (9.149.109.195) by e06smtp04.uk.ibm.com (192.168.101.134) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Fri, 6 Sep 2019 14:00:08 +0100 Received: from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com [9.149.105.58]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id x86D07JM54722672 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 6 Sep 2019 13:00:08 GMT Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id D3D064C05C; Fri, 6 Sep 2019 13:00:07 +0000 (GMT) Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id B967F4C04A; Fri, 6 Sep 2019 13:00:07 +0000 (GMT) Received: from oc3748833570.ibm.com (unknown [9.145.78.39]) by d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP; Fri, 6 Sep 2019 13:00:07 +0000 (GMT) Received: by oc3748833570.ibm.com (Postfix, from userid 1000) id 35504D802DF; Fri, 6 Sep 2019 15:00:07 +0200 (CEST) Subject: Re: [PATCH] Use type alignment in get_builtin_sync_mem To: richard.guenther@gmail.com (Richard Biener) Date: Fri, 06 Sep 2019 13:01:00 -0000 From: "Ulrich Weigand" Cc: gcc-patches@gcc.gnu.org (GCC Patches) In-Reply-To: from "Richard Biener" at Sep 06, 2019 12:46:09 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit x-cbid: 19090613-0016-0000-0000-000002A7851E X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 19090613-0017-0000-0000-00003307FEA5 Message-Id: <20190906130007.35504D802DF@oc3748833570.ibm.com> X-SW-Source: 2019-09/txt/msg00383.txt.bz2 Richard Biener wrote: > On Tue, Sep 3, 2019 at 3:09 PM Ulrich Weigand wrote: > > > If you read the C standards fine-print then yes. But people (or > > > even the C frontend!) hardly get that correct since for example > > > for > > > > > > struct __attribute__((packed)) { int i; } s; > > > > > > void foo () > > > { > > > __builtin_printf ("%p", &s.i); > > > } > > > > > > the C fronted actually creates a int * typed pointer for the ADDR_EXPR > > > (and not an int * variant with 1-byte alignment). And people use > > > int * to pass such pointers everywhere. > > > > Well ... I'd say if you cast to int * and then use an atomic operation, > > it's your own fault that it fails :-/ If the frontend itself uses > > the wrong type, that is of course a problem. > > Yes. The C standard says that if you cast something to a pointer type > the pointer has to be aligned according to the pointed-to type, otherwise > undefined. But we have no chance to use this because of this kind of > issues (and of course developer laziness...). But as far as I can see, for *atomic* operations at least, we do make that assumption. The x86 back-end for example just assumes that any "int" or "long" object that is the target of an atomic operation is naturally aligned, or else the generated code would just crash. So if you used your example with a packed struct and passed that pointer to an atomic, the back-end would still assume the alignment and the code would crash. But I'd still consider this a perfectly reasonable and expected behavior in this case ... The only thing that is special on s390 is that for the 16-byte integer type, the "natural" (mode) alignment is only 8 bytes, but for atomics we require 16 bytes. But if you explicitly use a 16-byte aligned pointer type to assert to the compiler that this object *is* aligned, the compiler should be able to rely on that. Of course if some part of the middle end get the alignment wrong, we have a problem. But I'm not sure how this could happen here. I agree that it might be the case that a user-specified *under*-alignment might get lost somewhere (e.g. you have a packed int and somewhere in the optimizers this gets accidentally cast to a normal int with the default alignment). But in *this* case, what would have to happen is that the middle-end accidentally casts something to a pointer with *higher* than the default alignment for the type, even though no such pointer cast is present in the source. Can this really happen? > I'm not sure how it is done now but IIRC the users > use __atomic_load (ptr) and then the frontend changes that > to one of BUILT_IN_ATOMIC_LOAD_{N,1,2,4,8,16} based on > some criteria (size mainly). I'm saying we should factor in > alignment here, _not_ using say BUILT_IN_ATOMIC_LOAD_16 > if according to the C standard the data isn't aligned. Maybe we can > ask the target whether the alignment according to C matches the > requirement for _16 expansion. And if things are not fine > the FE should instead use BUILT_IN_ATOMIC_LOAD_N > which we later if the compiler can prove bigger alignment and N > is constant could expand as one of the others. > > But safety first. The problem with using the _N variant is that we then get a call to the _n version of the library routine, right? This would actually be wrong on s390. The problem is that all atomic operations on any one single object need to be consistent: they either all use the 16-byte atomic instruction, or else they all protect the access with a lock. If you have parts of the code use the lock and other parts use the instruction, they're not actually protected against each other. This is why the _16 version of the library routine does the runtime alignment check, so that all accesses to actually 16-byte aligned objects use the instruction, both in the library and in compiler- generated code. The _n version doesn't do that. So I guess we'd have to convert the _n version back to the _16 library call if the size is constant 16, or something? > So yes, I'd say try tackling the issue in the frontend which is the > last to know the alignment in the most optimistic way (according > to the C standard). At RTL expansion time we have to be (very) > conservative. > > Btw, the alternative is to add an extra alignment parameter > to all of the atomic builtins where the FE could stick the alignment > for us to use during RTL expansion. But given we have the _{1,2,4,8,16,N} > variants it sounds easier to use those... > > Note we only document __atomic_load and __atomic_load_n so > the _{1,2,4,8,16} variants seem to be implementation detail. Adding the alignment parameter would work, I think. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com