[PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Hal Finkel via llvm-commits]

On 08/09/2017 10:56 PM, Xinliang David Li wrote:
>
>
> On Wed, Aug 9, 2017 at 8:37 PM, Hal Finkel <hfinkel at anl.gov 
> <mailto:hfinkel at anl.gov>> wrote:
>
>
>     On 08/09/2017 10:14 PM, Xinliang David Li via llvm-commits wrote:
>>
>>
>>     On Wed, Aug 9, 2017 at 7:42 PM, Chandler Carruth via Phabricator
>>     <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
>>
>>         chandlerc added a comment.
>>
>>         This has been discussed before and I still pretty strongly
>>         disagree with it.
>>
>>         This cripples the ability of TSan to find race conditions
>>         between accesses to consecutive bitfields -- and these bugs
>>         have actually come up.
>>
>>
>>
>>     Can you elaborate on this? Do you mean race conditions due to
>>     widening?
>>
>>         We also have had cases in the past where LLVM missed
>>         significant bitfield combines and optimizations due to
>>         loading them as separate integers. Those would become
>>         problems again, and I think they would be even harder to
>>         solve than narrowing the access is going to be because we
>>         will have strictly less information to work with.
>>
>>
>>      Can you elaborate here too? If there were missed optimization
>>     that later got fixed, there should be regression tests for them,
>>     right?  And what information is missing?
>
>     To make a general statement, if we load (a, i8) and (a+2, i16),
>     for example, and these came from some structure, we've lost the
>     information that the load (a+1, i8) would have been legal (i.e. is
>     known to be deferenceable). This is not specific to bit fields,
>     but the fact that we lose information on the dereferenceable byte
>     ranges around memory access turns into a problem when we later
>     can't legally widen. There may be a better way to keep this
>     information other than producing wide loads (which is an imperfect
>     mechanism, especially the way we do it by restricting to legal
>     integer types), but at the moment, we don't have anything better.
>
>
> Ok, as you mentioned, widening looks like a workaround to paper over 
> the weakness in IR to annotate the information.  More importantly, my 
> question is whether this is a just theoretical concern.

The issue is not theoretical in general. Thinking about it, we may 
actually have enough information in TBAA metadata, although we don't use 
it that way currently.

>
> If this is an important issue to avoid before we have a good IR, it 
> seems that we can always safely do this if there is no 'gap' between 
> the bitfield candidates.

I doubt it's that simple. The problem is that the widening happens late, 
and the frontend has no way of knowing what accesses will be generated, 
and which will survive late enough to help the widening phase. Some of 
the access the frontend generates might turn out to be dead, for 
example, or redundant, even though we actually want to essentially 
perform them anyway because of widening.

  -Hal

>
> David
>
>
>      -Hal
>
>>
>>     thanks,
>>
>>     David
>>
>>
>>         Ultimately, while I understand the appeal of this approach, I
>>         don't think it is correct and I think we should instead
>>         figure out how to optimize these memory accesses well in
>>         LLVM. That approach will have the added benefit of optimizing
>>         cases where the user has manually used a large integer to
>>         simulate bitfields, and making combining and canonicalization
>>         substantially easier.
>>
>>
>>         Repository:
>>           rL LLVM
>>
>>         https://reviews.llvm.org/D36562 <https://reviews.llvm.org/D36562>
>>
>>
>>
>>
>>
>>
>>     _______________________________________________
>>     llvm-commits mailing list
>>     llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>     <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
>     -- 
>     Hal Finkel
>     Lead, Compiler Technology and Programming Languages
>     Leadership Computing Facility
>     Argonne National Laboratory
>
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170809/46b620af/attachment.html>