[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

Tue Aug 22 19:32:34 PDT 2017

On 08/22/2017 09:18 PM, Xinliang David Li via llvm-commits wrote:
>
>
> On Tue, Aug 22, 2017 at 7:10 PM, Chandler Carruth via llvm-commits 
> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>
>     On Tue, Aug 22, 2017 at 7:03 PM Xinliang David Li via cfe-commits
>     <cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>>
>     wrote:
>
>         On Tue, Aug 22, 2017 at 6:37 PM, Chandler Carruth via
>         Phabricator <reviews at reviews.llvm.org
>         <mailto:reviews at reviews.llvm.org>> wrote:
>
>             chandlerc added a comment.
>
>             I'm really not a fan of the degree of complexity and
>             subtlety that this introduces into the frontend, all to
>             allow particular backend optimizations.
>
>             I feel like this is Clang working around a fundamental
>             deficiency in LLVM and we should instead find a way to fix
>             this in LLVM itself.
>
>             As has been pointed out before, user code can synthesize
>             large integers that small bit sequences are extracted
>             from, and Clang and LLVM should handle those just as well
>             as actual bitfields.
>
>             Can we see how far we can push the LLVM side before we add
>             complexity to Clang here? I understand that there remain
>             challenges to LLVM's stuff, but I don't think those
>             challenges make *all* of the LLVM improvements off the
>             table, I don't think we've exhausted all ways of improving
>             the LLVM changes being proposed, and I think we should
>             still land all of those and re-evaluate how important
>             these issues are when all of that is in place.
>
>
>         The main challenge of doing  this in LLVM is that
>         inter-procedural analysis (and possibly cross module) is
>         needed (for store forwarding issues).
>
>         Wei, perhaps you can provide concrete test case to illustrate
>         the issue so that reviewers have a good understanding.
>
>
>     It doesn't seem like all options for addressing that have been
>     exhausted. And even then, I feel like trying to fix this with
>     non-obvious (to the programmer) frontend heuristics isn't a good
>     solution. I actually *prefer* the source work around of "don't use
>     a bitfield if you *must* have narrow width access across modules
>     where the optimizer cannot see enough to narrow them and you
>     happen to know that there is a legal narrow access that works".
>     Because that way the programmer has *control* over this rather
>     than being at the whim of whichever side of the heuristic they end
>     up on.
>
>
>
> The source workaround solution *does not* scale. Most importantly, 
> user may not even be aware of the problem (and performance loss) 
> unless  compiling the code with another compiler and notice the 
> performance difference.

I agree with this, but it's not clear that this has to scale in that 
sense. I don't like basing this on the bitfield widths because it makes 
users pick between expressing semantic information and expressing target 
tuning information using the same construct. What if the optimal answer 
here is different on different platforms? I don't want to encourage 
users to ifdef their aggregates to sometimes be bitfields and sometimes 
not for tuning reasons. If need be, please add an attribute. Any 
heuristic that you pick here is going to help some cases and hurt 
others. If we're at the level of needing IPA to look at store-to-load 
forwarding effects, then we've really already lost. Either you need to 
actually do the IPA, or even in the backend, any heuristic that you 
choose will help some things and hurt others. Hopefully, we're not 
really there yet. I'm looking forward to seeing more examples of the 
kinds of problems you're trying to solve.

Thanks again,
Hal

>
> David
>
>
>         David
>
>
>
>             Repository:
>               rL LLVM
>
>             https://reviews.llvm.org/D36562
>             <https://reviews.llvm.org/D36562>
>
>
>
>         _______________________________________________
>         cfe-commits mailing list
>         cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
>         http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>         <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
>
>
>     _______________________________________________
>     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>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-- 
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/20170822/0a375518/attachment.html>