[PATCH] D16586: Make clang AAPCS compliant w.r.t volatile bitfield accesses

[Asiri Rathnayake via cfe-commits]

rmaprath updated this revision to Diff 46264.
rmaprath added a comment.

Addressing review comments by @rjmccall:

Moved all the AAPCS specific tweaks to EmitLValueForField(), this simplified the patch a lot (now there is no mucking about a de-constructed GEP at load/store points). In order to do this, LValue definition had to be updated so that it gets initialized with is own copy of CGBitFieldInfo.

> A few high-level notes:

>  AAPCS requires the bit-field to be loaded on a store, even if the store fills the entire container; that doesn't seem to be implemented in your patch.


Fixed.

> Especially because of #1, let's not do this unless the l-value is actually volatile. The ABI rule here is arguably actively wrong for non-volatile cases, e.g. struct { volatile char c; short s : 8; }.


The AAPCS also say (Section 7.1.7.5):

"If the container of a non-volatile bit-field overlaps a volatile bit-field then it is undefined whether access to the non volatile field will cause the volatile field to be accessed."

So it looks like doing this for normal fields is still within the bounds of AAPCS. In fact, armcc loads the volatile 'c' in your example when 's' is accessed. But I agree that limiting this to volatile cases is a good idea; we are still AAPCS compliant and there's less things to break.

> Instead of using string comparisons all over the place, please make this a flag on the CG TargetInfo or something.


Factored this out into a small utility function.


http://reviews.llvm.org/D16586

Files:
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGValue.h
  lib/CodeGen/CodeGenFunction.h
  test/CodeGen/aapcs-bitfield.c

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D16586.46264.patch
Type: text/x-patch
Size: 23084 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160128/480b187e/attachment-0001.bin>