[PATCH] [OPENMP] CodeGen for "omp atomic read [seq_cst]" directive.

[Bataev, Alexey]

John, thanks for the review. I prepared a new patch, please look at this.

Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team

16.01.2015 13:01, John McCall пишет:
> ================
> Comment at: lib/CodeGen/CGAtomic.cpp:77
> @@ +76,3 @@
> +        auto &OrigBFI = lvalue.getBitFieldInfo();
> +        auto OffsetInChars = C.toCharUnitsFromBits(OrigBFI.Offset);
> +        auto VoidPtrAddr = CGF.EmitCastToVoidPtr(lvalue.getBitFieldAddr());
> ----------------
> Hmm.  I feel like you should make sure that your rule uses an aligned atomic access if it's possible to do so.  That is, if the bitfield does fall within a single aligned unit, you should definitely access it with an atomic of that size.  I think you need to consider the actual bitfield width for this, but maybe I'm missing something and that's done implicitly elsewhere.
>
> ================
> Comment at: lib/CodeGen/CGAtomic.cpp:86
> @@ +85,3 @@
> +        BFI.Offset %= C.getCharWidth();
> +        BFI.StorageSize = AtomicSizeInBits;
> +        LVal = LValue::MakeBitfield(Addr, BFI, lvalue.getType(),
> ----------------
> This is dangerous; I think you can end up with an access that goes beyond the end of the structure with this.  Consider a 1-bit bitfield of type "unsigned" that's at the end of the struct, e.g.
>
> struct {
>    unsigned : 31
>    unsigned x : 1; // <- access is to this
> };
>
> You need to make sure this is only accessed with an 8-bit operation.
>
> ================
> Comment at: lib/CodeGen/CGAtomic.cpp:88
> @@ +87,3 @@
> +        LVal = LValue::MakeBitfield(Addr, BFI, lvalue.getType(),
> +                                    lvalue.getAlignment());
> +      } else if (lvalue.isVectorElt()) {
> ----------------
> This alignment needs to be adjusted.
>
> http://reviews.llvm.org/D6431
>
> EMAIL PREFERENCES
>    http://reviews.llvm.org/settings/panel/emailpreferences/
>
>