[cfe-commits] PATCH: Large re-working of bitfield IR-gen, and a fix for PR13691

[Chandler Carruth]

I've added an explanatory comment. Hopefully won't get stale.

Any other comments? I'd love to get this into the tree as we're seeing
actual races in the wild due to it....


On Tue, Aug 28, 2012 at 11:01 AM, Sean Silva <silvas at purdue.edu> wrote:

> The tests and the comment serve different purposes.
>
> The tests are just machine-checkable examples of the expected behavior
> written in a way amenable to being checked by the machine. The comment
> is written with the reader in mind, with the express purpose of
> communicating an understanding of what the code does; it can use the
> full power of natural language and other human-understandable
> notations (like pseudo-IR) to convey this.
>
> As for fearing that the comment will fall out of date. I think it's
> safe to say that the "big picture" is not going to change
> significantly without causing enough churn in the source code that the
> comment gets fixed. The current patch is a data point supporting this.
> You can also mitigate the problem by cross-referencing specific tests
> from the comment---incongruities between the test and the description
> will alert the reader that the comment has fallen out of date.
>
> --Sean Silva
>
> On Tue, Aug 28, 2012 at 1:18 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
> > On Tue, Aug 28, 2012 at 10:13 AM, Sean Silva <silvas at purdue.edu> wrote:
> >>
> >> -class CGBitFieldInfo {
> >> -public:
> >> -  /// Descriptor for a single component of a bit-field access. The
> entire
> >> -  /// bit-field is constituted of a bitwise OR of all of the individual
> >> -  /// components.
> >> -  ///
> >> -  /// Each component describes an accessed value, which is how the
> >> component
> >> -  /// should be transferred to/from memory, and a target placement,
> >> which is how
> >> -  /// that component fits into the constituted bit-field. The pseudo-IR
> >> for a
> >> -  /// load is:
> >> -  ///
> >> -  ///   %0 = gep %base, 0, FieldIndex
> >> -  ///   %1 = gep (i8*) %0, FieldByteOffset
> >> -  ///   %2 = (i(AccessWidth) *) %1
> >> -  ///   %3 = load %2, align AccessAlignment
> >> -  ///   %4 = shr %3, FieldBitStart
> >> -  ///
> >> -  /// and the composed bit-field is formed as the boolean OR of all
> >> accesses,
> >> -  /// masked to TargetBitWidth bits and shifted to TargetBitOffset.
> >>
> >> Could you beef up the 2-liner comment you replaced this nice comment
> >> with to include the pseudo-IR for a bitfield access like this comment
> >> does? I think that's a very valuable piece of documentation to have.
> >
> >
> > Is it really a good idea to have this in the comment versus the tests? My
> > far is that the comment will slip out of date.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120828/832c26b7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR13691.patch
Type: application/octet-stream
Size: 59003 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120828/832c26b7/attachment.obj>