<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 9, 2017 at 7:42 PM, Chandler Carruth via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc added a comment.<br>
<br>
This has been discussed before and I still pretty strongly disagree with it.<br>
<br>
This cripples the ability of TSan to find race conditions between accesses to consecutive bitfields -- and these bugs have actually come up.<br>
<br></blockquote><div><br></div><div><br></div><div>Can you elaborate on this? Do you mean race conditions due to widening?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br></blockquote><div><br></div><div> 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?</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D36562" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36562</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div></div>