[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

tstellar added a comment. This introduced a build failure on s390x: https://github.com/llvm/llvm-project/issues/69146 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143813/new/ https://reviews.llvm.org/D143813 ___ cfe-commits mailing list

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

jonpa accepted this revision. jonpa added a comment. This revision is now accepted and ready to land. In D143813#4269073 , @efriedma wrote: > Please don't upload reviews for followup patches on top of the original > patch; it confuses the history of

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

efriedma added a comment. Please don't upload reviews for followup patches on top of the original patch; it confuses the history of happened when. But sure, looks fine. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143813/new/ https://reviews.llvm.org/D143813

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

jonpa updated this revision to Diff 513573. jonpa added a comment. > Should be straightforward to fix, though; just make CheckAtomicAlignment > return the computed pointer. Something like this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143813/new/ https://reviews.llvm.org/D143813

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

jonpa added a comment. In D143813#4258744 , @ahatanak wrote: > In D143813#4257943 , @jonpa wrote: > >> I don't understand the first argument - I thought it was supposed to be just >> an address... > > It's a

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

ahatanak added a comment. In D143813#4257943 , @jonpa wrote: > I don't understand the first argument - I thought it was supposed to be just > an address... It's a statement expression. https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html The value

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

efriedma added a comment. I think the issue is that you're calling EmitPointerWithAlignment() on the argument, then calling EmitScalarExpr on the same argument. Essentially, emitting the argument twice. If emitting the argument has side-effects, that will cause an issue. Sorry, should have

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

jonpa added a comment. In D143813#4256969 , @ahatanak wrote: > I think this patch is causing the assertion in > `CodeGenFunction::setAddrOfLocalVar` to fail when the following code is > compiled: > > void foo(unsigned long long t) { >

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

ahatanak added a comment. I think this patch is causing the assertion in `CodeGenFunction::setAddrOfLocalVar` to fail when the following code is compiled: void foo(unsigned long long t) { __sync_bool_compare_and_swap(({int x = 1; }), t, t); } Could you take a look? Repository: rG

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

srj added a comment. > (I certainly agree everyone should be using __atomic and not __sync.) Halide still uses the `__sync` builtins for 32-bit targets, for reasons I don't have at hand now (IIRC there were downstream users with tooling issues, I will re-check). (I don't think we are using

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

This revision was automatically updated to reflect the committed changes. Closed by commit rG7501e53b8d6d: [Clang] Give warning for an underaligned 128-bit __sync library call. (authored by jonpa). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM