[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:396
+/// according to the field declaring type width.
+CODEGENOPT(ForceNoAAPCSBitfieldWidth, 1, 0)
+
----------------
dnsampaio wrote:
> ostannard wrote:
> > Why is this a negative option, when the one above is positive?
> The enforcing of number of accesses would not be accepted if it was not an opt-in option. This one I expect it should be accepted with a single opt-out option.
My problem is with the name of the option (adding an extra negative just makes things more confusing), not with the default value. This could just be called `AAPCSBitfieldWidth`, (unless you think the `Force` is adding something), and default to true father than false.


================
Comment at: clang/include/clang/Driver/Options.td:2328
   HelpText<"Follows the AAPCS standard that all volatile bit-field write generates at least one load. (ARM only).">;
+def ForceNoAAPCSBitfieldWidth : Flag<["-"], "fno-AAPCSBitfieldWidth">, Group<m_arm_Features_Group>,
+  Flags<[DriverOption,CC1Option]>,
----------------
ostannard wrote:
> Command-line options are in kebab-case, so this should be something like `fno-aapcs-bitfield-width`. This also applies to the `fAAPCSBitfieldLoad` option above, assuming it's not too late to change that.
> 
> Please also add a positive version of this option (i.e. `faapcs-bitfield-width`).
This still needs a positive version.


================
Comment at: clang/lib/CodeGen/CGRecordLayout.h:83
 
+  /// The offset within a contiguous run of bitfields that are represented as
+  /// a single "field" within the LLVM struct type. This offset is in bits.
----------------
These doc comments are copied from the ones above, they need changing.


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:279
+    lowerUnion();
+    return computeVolatileBitfields();
+  }
----------------
Returning `void` is confusing (yes I know it was already there), this should be a separate `return;` statement.


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:288
+      appendPaddingBytes(Size);
+      return computeVolatileBitfields();
+    }
----------------
Same here.


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:535
+///
+/// Enforcing the width restriction can be disable using
+/// -fno-aapcs-bitfield-width.
----------------
s/disable/disabled/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72932/new/

https://reviews.llvm.org/D72932