[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. In D76384#1986761 , @mibintc wrote: > In D76384#1986525 , @mibintc wrote: > > > @rjmccall Can you check the patch added last night here, commit > >

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. I also added this patch which is a companion to this. commit 8812b0cc5cc09f350d8e89bff99f185c5e1a5d4d Author: Melanie Blower Date: Thu Apr 16 08:45:26 2020 -0700 [NFC] Rename Sema.FPFeatures to

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. In D76384#1986525 , @mibintc wrote: > @rjmccall Can you check the patch added last night here, commit > 3ee1ec0b9dd6ee2350f39ae8a418bf3ce28d06cf > > Author:

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc marked an inline comment as done. mibintc added a subscriber: martong. mibintc added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:6821 + E->getFPFeatures(Importer.getFromContext()), + importChecked(Err, ToComputationLHSType), +

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:6821 + E->getFPFeatures(Importer.getFromContext()), + importChecked(Err, ToComputationLHSType), + importChecked(Err, ToComputationResultType)); This introduced an assertion

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. @rjmccall Can you check the patch added last night here, commit 3ee1ec0b9dd6ee2350f39ae8a418bf3ce28d06cf Author: Benjamin Kramer Date: Thu Apr 16 11:45:02 2020 +0200 LangOptions cannot depend on

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. In D76384#1984683 , @mibintc wrote: > In D76384#1984584 , @rjmccall wrote: > > > I *would* like an NFC patch first that renames `FPFeatures` to something > > like `CurFPFeatures` in order

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

This revision was automatically updated to reflect the committed changes. Closed by commit rG2ba4e3a4598b: Move BinaryOperators.FPOptions to trailing storage (authored by mibintc). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. In D76384#1984584 , @rjmccall wrote: > I *would* like an NFC patch first that renames `FPFeatures` to something like > `CurFPFeatures` in order to more clearly distinguish it from `FPOptions` in > Sema code, though. That should

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. In D76384#1984584 , @rjmccall wrote: > In D76384#1984498 , @mibintc wrote: > > > Responding to @rjmccall 's review. John, after this is approved I want to > > proceed with pragma

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Oh, and this patch LGTM, thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/ https://reviews.llvm.org/D76384

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. In D76384#1984498 , @mibintc wrote: > Responding to @rjmccall 's review. John, after this is approved I want to > proceed with pragma float_control as proposed in D72841 > . Can you recommend

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 257789. mibintc added a comment. Responding to @rjmccall 's review. John, after this is approved I want to proceed with pragma float_control as proposed in D72841 . Can you recommend an approach, do you think I will need

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. Minor fixes, then LGTM. Comment at: clang/lib/AST/ASTImporter.cpp:6807 + importChecked(Err, ToComputationLHSType), + importChecked(Err, ToComputationResultType)); } Oh, these two calls need to use

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 257704. mibintc added a comment. Lost power in Monday's storm, back online today. I made the changes requested by @rjmccall. Look OK? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/lib/Analysis/BodyFarm.cpp:120 +VK_RValue, OK_Ordinary, SourceLocation(), +FPOptions::defaultWithoutTrailingStorage(C)); } mibintc wrote: > rjmccall

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc marked 3 inline comments as done. mibintc added a comment. Adding an inline reply for John. rebased the patch, also re-applied clang-format. What do you think? Comment at: clang/lib/Analysis/BodyFarm.cpp:120 +VK_RValue, OK_Ordinary,

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 257055. mibintc added a comment. Responded to @rjmccall 's review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/ https://reviews.llvm.org/D76384 Files: clang/include/clang/AST/Expr.h

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. Thanks. Just a bunch of fairly minor requests now. Comment at: clang/include/clang/AST/Expr.h:3474 + size_t offsetOfTrailingStorage() const; + size_t offsetOfTrailingStorage(); + You don't need the non-const overload.

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 256998. mibintc added a comment. I made the changes requested by @rjmccall ; I also used clang-format on the tip. check-clang is passing. Look OK? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. Okay, that's fine, it's definitely a simpler change to not mess with the basic node hierarchy. The one problem I see is that you've (accidentally?) set up the `FPOptions` methods so that you can't call them on a `BinaryOperator` that's actually a

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 256648. mibintc added a comment. I finally decided that combining BinaryOperator and CompoundAssignOperator was too difficult, this patch uses the trailing object approach similar to that used in CallExpr. @rjmccall thank you once again for all your reviews

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. I would guess that some visitor still has a `VisitCompoundAssignOperator` implementation, and that the behavior of `VisitBinAssign` happens to do what's needed for it. You basically have two options: - Keep a `VisitCompoundAssignOperator` around in `StmtVisitor` for

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. It seems like the macro's in StmtVisitor.h only work if CAO and BinaryOperator are separate classes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/ https://reviews.llvm.org/D76384

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

sepavloff added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + rjmccall wrote: > sepavloff wrote: > > rjmccall wrote: > > > sepavloff wrote: > > > > erichkeane wrote: > > > > > rjmccall wrote: > > > > > >

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. If I change StmtVisitor to send compound assignment opcodes to BinaryOperator, then 12 clang LIT tests fail, all related to compound assignment mishandling, i.e. this patch - a/clang/include/clang/AST/StmtVisitor.h +++ b/clang/include/clang/AST/StmtVisitor.h @@ -143,7

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 255301. mibintc added a comment. I beleive this patch responds to all @rjmccall 's review comments, except I don't know how to program a solution to his StmtVisitor remark. I'll add more info about that. This patch optionally allocates the trailing storage

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + sepavloff wrote: > rjmccall wrote: > > sepavloff wrote: > > > erichkeane wrote: > > > > rjmccall wrote: > > > > > erichkeane wrote: > > > > > >

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

sepavloff added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + rjmccall wrote: > sepavloff wrote: > > erichkeane wrote: > > > rjmccall wrote: > > > > erichkeane wrote: > > > > > rjmccall wrote: > > > > > >

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + sepavloff wrote: > erichkeane wrote: > > rjmccall wrote: > > > erichkeane wrote: > > > > rjmccall wrote: > > > > > rjmccall wrote: > > > > > >

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

sepavloff added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + erichkeane wrote: > rjmccall wrote: > > erichkeane wrote: > > > rjmccall wrote: > > > > rjmccall wrote: > > > > > erichkeane wrote: > > > > > >

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

erichkeane added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + rjmccall wrote: > erichkeane wrote: > > rjmccall wrote: > > > rjmccall wrote: > > > > erichkeane wrote: > > > > > rjmccall wrote: > > > > > > The

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + erichkeane wrote: > rjmccall wrote: > > rjmccall wrote: > > > erichkeane wrote: > > > > rjmccall wrote: > > > > > The problem with having both

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

erichkeane added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + rjmccall wrote: > rjmccall wrote: > > erichkeane wrote: > > > rjmccall wrote: > > > > The problem with having both functions that take

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + rjmccall wrote: > erichkeane wrote: > > rjmccall wrote: > > > The problem with having both functions that take `ASTContext`s and > > > functions

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/StmtVisitor.h:146 + RetTy VisitBin##NAME(PTR(BinaryOperator) S, ParamTys... P) { \ +DISPATCH(BinAssign, BinaryOperator); \ }

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

erichkeane added a comment. Trying to help Melanie respond to the comments, so I ran through the patch and came up with the following comments/responses. Sorry for the 'surprise hit' :) Comment at: clang/include/clang/AST/Expr.h:3474 + static unsigned

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:3474 + static unsigned sizeOfTrailingObjects(bool hasFP, bool isCompound) { +return (hasFP ? 1 : 0) * sizeof(unsigned) + + (isCompound ? 2 : 0) * sizeof(QualType); Sorry, I

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 253983. mibintc added a comment. Here's another revision, I believe this handles all of @rjmccall 's requests. Thank you, John, for your review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2573 + FPOptions FPFeatures; + mibintc wrote: > rjmccall wrote: > > Let's split the CallExpr changes out into a separate patch, so that we have > > one patch that's *purely* about

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. On my RHEL8 system building Debug clang;clang-tools-extra, check-all is passing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/ https://reviews.llvm.org/D76384 ___

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 253341. mibintc marked 12 inline comments as done. mibintc added a comment. @rjmccall Many thanks for your code review. Please take a look at this when you get a chance. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc marked an inline comment as done. mibintc added a comment. I added some inline replies to John. I'm not certain I have everything yet exactly the way he wants it. Comment at: clang/include/clang/AST/Expr.h:2573 + FPOptions FPFeatures; + rjmccall

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:3465 +SubExprs[RHS] = rhs; +hasFPFeatures = true; +isCompoundAssign = 1; mibintc wrote: > rjmccall wrote: > > Okay, so this is *always* adding trailing storage. Is there a

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc marked an inline comment as done. mibintc added inline comments. Comment at: clang/include/clang/AST/Expr.h:3465 +SubExprs[RHS] = rhs; +hasFPFeatures = true; +isCompoundAssign = 1; rjmccall wrote: > Okay, so this is *always* adding trailing

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. I don't see those Harbormaster fails, using trunk and RHEL8 on architecture x86_64, Debug build of clang;clang-tools-extra and check-all. I ran all the failed lit tests separately and each passed Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2573 + FPOptions FPFeatures; + Let's split the CallExpr changes out into a separate patch, so that we have one patch that's *purely* about unifying BinaryOperator and

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

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

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 252469. mibintc added a comment. I think this addresses all the feedback from @rjmccall Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/ https://reviews.llvm.org/D76384 Files:

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. In D76384#1936769 , @mibintc wrote: > In D76384#1934785 , @rjmccall wrote: > > > Let's make this patch be purely a representation change. I wouldn't expect > > *any* test changes from

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc added a comment. In D76384#1934785 , @rjmccall wrote: > Let's make this patch be purely a representation change. I wouldn't expect > *any* test changes from that; if there are, we should understand why. You > can then add the stuff about

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. Let's make this patch be purely a representation change. I wouldn't expect *any* test changes from that; if there are, we should understand why. You can then add the stuff about tracking whether there's a pragma in a separate patch. Comment at:

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 251746. mibintc added a comment. This revision fixes the clang-tidy,clang-format and removes the redundant accessors. Ready for your review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76384/new/

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc marked 3 inline comments as done. mibintc added a comment. In D76384#1929774 , @mibintc wrote: > There's a bug in this patch that i haven't solved yet. I think it's just 1 > bug. The bug is that the AST statement visitor is going to the

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

mibintc updated this revision to Diff 251699. mibintc retitled this revision from "Move FPFeatures from BinaryOperator bitfields to Trailing storage - preliminary" to "Move FPFeatures from BinaryOperator bitfields to Trailing storage". mibintc added a comment. This revision has been rebased,

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage - preliminary

mibintc created this revision. mibintc added reviewers: rjmccall, sepavloff. Herald added subscribers: martong, jfb, arphaman. Herald added a reviewer: shafik. Herald added a project: clang. mibintc added a comment. There's a bug in this patch that i haven't solved yet. I think it's just 1 bug.

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage - preliminary

mibintc added a comment. There's a bug in this patch that i haven't solved yet. I think it's just 1 bug. The bug is that the AST statement visitor is going to the "fallback" instead of the right destination when walking CompoundAssignmentOperator. This patch collapses