[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

This revision was automatically updated to reflect the committed changes. Closed by commit rL372640: [Sema] Fix the atomic expr rebuilding order. (authored by hliao, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.l

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

hliao updated this revision to Diff 221375. hliao added a comment. update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67924/new/ https://reviews.llvm.org/D67924 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaChecking.cpp clang/l

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

hliao updated this revision to Diff 221374. hliao added a comment. add test case for compare_exchange. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67924/new/ https://reviews.llvm.org/D67924 Files: clang/include/clang/Sema/Sema.h clang/lib/Se

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

erichkeane added a comment. Seems your test changes have disappeared as well? Otherwise, 1 comment then I'm ok with this. Comment at: clang/lib/Sema/TreeTransform.h:3319 +Range, Range, RParenLoc, SubExprs, Op, +/*AtomicArgumentOrder*/ Sema::AtomicArgumentOrder

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

hliao updated this revision to Diff 221369. hliao added a comment. revised Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67924/new/ https://reviews.llvm.org/D67924 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaChecking.cpp clang/

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

erichkeane added a comment. Also, can you add a test for GNUCmpXchg in both situations (inside a template, and outside)? It is the most complicated, and would best reflect the issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67924/new/ https:/

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

erichkeane added a comment. It kinda stinks that we have to do this at such a late step. I'd have much preferred doing this as a part of the rebuild, but it appears that the 'form' takes quite a bit to calculate. Also, I'd likely have preferred that the initial reordering happened in codegen

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

hliao updated this revision to Diff 221365. hliao added a comment. Add parameter name for that default argument. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67924/new/ https://reviews.llvm.org/D67924 Files: clang/include/clang/Sema/Sema.h cl

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

hliao added a comment. In D67924#1679409 , @erichkeane wrote: > Yikes, good catch! > > Would we be better off instead to just modify how the other switch loads the > value? Presumably something like, "if (NeedsRearrangeArgs) > SubExprs.append(Args.begin

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

erichkeane added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:3319 +return getSema().BuildAtomicExpr(Range, Range, RParenLoc, SubExprs, Op, + true); } When passing a bool, use the comment syntax /*paramnam

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

erichkeane added a comment. Yikes, good catch! Would we be better off instead to just modify how the other switch loads the value? Presumably something like, "if (NeedsRearrangeArgs) SubExprs.append(Args.begin(), Args.end()); else /*the switch*/. Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

hliao created this revision. hliao added a reviewer: erichkeane. Herald added subscribers: cfe-commits, jfb. Herald added a project: clang. hliao added a comment. The current BuildAtomicExpr expects the arguments to be in the API order instead of the AST order. If RebuildAtomicExpr uses the same

[PATCH] D67924: [Sema] Fix the atomic expr rebuilding order.

hliao added a comment. The current BuildAtomicExpr expects the arguments to be in the API order instead of the AST order. If RebuildAtomicExpr uses the same BuildAtomicExpr, it needs to ensure the order of arguments are in API order; otherwise, arguments (especially the one with memory order) w