[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand closed this revision. uweigand added a comment. Committed as 4c5a93bd58bad70e91ac525b0e020bd5119a321a . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81583/new/ https://reviews.llvm.org/D81583

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81583/new/ https://reviews.llvm.org/D81583 ___ cfe-commits mailing list

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

asb added a comment. This LGTM from a RISC-V perspective. I'll likely follow up with a RISC-V test case similar to the SystemZ one post-commit, but given this is really fixing a cross-platform ABI issue this seems non-urgent. Thanks for spotting and addressing this issue. CHANGES SINCE LAST

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand updated this revision to Diff 276650. uweigand added a comment. Drop AllowNoUniqueAddress parameter; address review comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81583/new/ https://reviews.llvm.org/D81583 Files: clang/lib/CodeGen/TargetInfo.cpp

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

efriedma added a comment. In D81583#2139002 , @uweigand wrote: > In D81583#2137277 , @efriedma wrote: > > > I'm tempted to say this is a bugfix for the implementation of > > no_unique_address, and just fix it

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

rjmccall added a comment. I agree with Eli that this should be considered a bugfix in the implementation of a recent language change and should just be rolled out consistently for all targets. If this were a five-year-old feature, the ABI considerations would be different, but it's not.

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

qiucf added a comment. > In any case, I guess it would still be good to also have test cases for this > aspect of the ABI on Power ... Sure, I'd like to do pre-commit for PPC tests. (or after this landed and enable/remove `AllowNoUniqueAddr`) CHANGES SINCE LAST ACTION

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand added a comment. In D81583#2138127 , @qiucf wrote: > Thanks for this patch! > > If I understand correctly, only `isEmptyRecord`/`isEmptyField` and places > checking any field is zero bit-width may need change for this? Since >

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand marked 2 inline comments as done. uweigand added a comment. In D81583#2137277 , @efriedma wrote: > I'm tempted to say this is a bugfix for the implementation of > no_unique_address, and just fix it globally for all ABIs. We're never going > to

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand marked 6 inline comments as done. uweigand added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:521 + // [[no_unique_address]] attribute (since C++20). Those do count + // as empty according to the Itanium ABI. This property is currently + // only

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand updated this revision to Diff 276414. uweigand added a comment. Handle array of empty records correctly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81583/new/ https://reviews.llvm.org/D81583 Files: clang/lib/CodeGen/TargetInfo.cpp clang/test/CodeGen/systemz-abi.cpp

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

qiucf added a comment. Thanks for this patch! If I understand correctly, only `isEmptyRecord`/`isEmptyField` and places checking any field is zero bit-width may need change for this? Since `isSingleElementStruct` and `isHomogeneousAggregate` just use them to skip empty fields in aggregate. I

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

hubert.reinterpretcast added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:521 + // [[no_unique_address]] attribute (since C++20). Those do count + // as empty according to the Itanium ABI. This property is currently + // only respected if the

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

efriedma added a comment. I'm tempted to say this is a bugfix for the implementation of no_unique_address, and just fix it globally for all ABIs. We're never going to get anything done here if we require a separate patch for each ABI variant clang supports. Comment at:

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand updated this revision to Diff 276151. uweigand added a comment. Rebased against mainline. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81583/new/ https://reviews.llvm.org/D81583 Files: clang/lib/CodeGen/TargetInfo.cpp clang/test/CodeGen/systemz-abi.cpp Index:

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

hubert.reinterpretcast added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:523 + if (isa(RT->getDecl())) { +if (!(AllowNoUniqueAddr && FD->hasAttr())) + return false; Minor nit: The nested `if` could be merged with the outer one: ```

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand added a comment. Another ping ... See also http://lists.llvm.org/pipermail/cfe-dev/2020-July/066162.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81583/new/ https://reviews.llvm.org/D81583

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand added a comment. Ping? I'd really appreciate feedback about this ABI issue, in particular from other affected target maintainers ... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81583/new/ https://reviews.llvm.org/D81583

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

uweigand created this revision. uweigand added reviewers: craig.topper, erichkeane, jasonliu, kbarton, rnk, asl, sunfish, t.p.northover, arsenm, asb. Herald added subscribers: cfe-commits, wdng. Herald added a project: clang. The SystemZ ABI has special cases to handle structs containing just a