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

ostannard accepted this revision. ostannard added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___ cfe-commits mailing list

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

ostannard added inline comments. Comment at: clang/test/CodeGen/volatile.c:1 -// RUN: %clang_cc1 -triple=%itanium_abi_triple -emit-llvm < %s | FileCheck %s -check-prefix CHECK -check-prefix CHECK-IT +// RUN: %clang_cc1 -triple=%itanium_abi_triple -emit-llvm < %s | FileCheck %s

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

stuij reopened this revision. stuij added a comment. This revision is now accepted and ready to land. Reopening as this commit made clang/test/CodeGen/volatile.c fail on Arm/AArch64 buildbot hosts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

ostannard accepted this revision. ostannard added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932

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

stuij marked 6 inline comments as done. stuij added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.def:396 +/// according to the field declaring type width. +CODEGENOPT(ForceNoAAPCSBitfieldWidth, 1, 0) + ostannard wrote: > dnsampaio wrote:

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

stuij commandeered this revision. stuij added a reviewer: dnsampaio. stuij added a comment. Commandeering as I've made some changes to the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932

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

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,

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

stuij added a comment. @ostannard: pinging on behalf of @dnsampaio. The changes still apply cleanly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___

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

dnsampaio added a comment. Ping ... ping... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

dnsampaio added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

dnsampaio marked 6 inline comments as done. dnsampaio added a comment. Indeed not all of them. Fixed this time. Comment at: clang/include/clang/Basic/CodeGenOptions.def:396 +/// according to the field declaring type width. +CODEGENOPT(ForceNoAAPCSBitfieldWidth, 1, 0) +

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

ostannard added a comment. You haven't addressed my earlier inline comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___ cfe-commits mailing list

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

dnsampaio added a comment. Herald added a subscriber: dang. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___ cfe-commits mailing list

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

dnsampaio added a comment. Hi @ostannard, thanks for your review. I updated the patch so it won't act when the computed volatile bit-field access will overlap a zero length bit-field, avoiding the conflict. We can update it accordingly to future versions of the AAPCS if required. Repository:

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

ostannard added a comment. I think I've spotted a bug in the ABI spec, which you've faithfully implemented here. I don't know of any other compiler which has implemented this ABI change yet, so it's probably worth seeing if we can get the spec fixed. The intention of the ABI is to avoid

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

dnsampaio updated this revision to Diff 243197. dnsampaio added a comment. Added opt-out flag Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 Files: clang/include/clang/Basic/CodeGenOptions.def

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

dnsampaio planned changes to this revision. dnsampaio added a comment. Updated wrong patch here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___

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

dnsampaio updated this revision to Diff 242823. dnsampaio added a comment. Herald added subscribers: llvm-commits, hiraditya. Herald added a project: LLVM. - Removed test - Added clear at the end of run as well, to clear waste - Moved clearing to a more sensible position Repository: rG LLVM

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

dnsampaio added a comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

dnsampaio added a comment. Ping :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

dnsampaio updated this revision to Diff 241477. dnsampaio added a comment. - Do not generate special volatile access if the record alignment is smaller than the bit-field declared type Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/

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

dnsampaio updated this revision to Diff 241421. dnsampaio added a comment. Herald added a subscriber: jfb. - Moved computation of volatile accesses to the record layout builder Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/

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

ostannard added a comment. > It will require changing all possible initializations, with a sensible value. Where are you seeing multiple initializations? It looks like all of the logic for struct layout happens in `CGRecordLowering::lower`, we might need to add a pass there to calculate the

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

dnsampaio added a comment. In D72932#1829716 , @ostannard wrote: > Why are you doing this in CodeGen, rather than adjusting the existing layout > code in CGRecordLowering? Doing it this way will result in > AdjustAAPCSBitfieldLValue being called for

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

dnsampaio added a comment. In D72932#1829716 , @ostannard wrote: > Why are you doing this in CodeGen, rather than adjusting the existing layout > code in CGRecordLowering? Doing it this way will result in > AdjustAAPCSBitfieldLValue being called for

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

dnsampaio reopened this revision. dnsampaio added a comment. Sorry, submitted using ide by mistake. Already reverted it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72932/new/ https://reviews.llvm.org/D72932

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG6a24339a4524: [ARM] Follow AACPS standard for volatile bit-fields access width (authored by dnsampaio). Repository: rG

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

dnsampaio added a comment. In D72932#1829716 , @ostannard wrote: > Why are you doing this in CodeGen, rather than adjusting the existing layout > code in CGRecordLowering? Doing it this way will result in > AdjustAAPCSBitfieldLValue being called for

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

ostannard added a comment. Why are you doing this in CodeGen, rather than adjusting the existing layout code in CGRecordLowering? Doing it this way will result in AdjustAAPCSBitfieldLValue being called for every access to the bitfield, rather than just once. This is probably more fragile too,

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

dnsampaio created this revision. dnsampaio added reviewers: rsmith, rjmccall. Herald added subscribers: cfe-commits, kristof.beyls. Herald added a project: clang. This patch resumes the work of D16586 . According to the AAPCS, volatile bit-fields should be