Route memory-builtin recognition through AllocCallInfo. NFC - #1935
Conversation
57ef130 to
e1c50f3
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // one place instead of ad-hoc name checks scattered across the visitor. | ||
| class AllocCallInfo { | ||
| public: | ||
| enum class Kind { None, Malloc, Calloc, Realloc, Free }; |
There was a problem hiding this comment.
warning: enum 'Kind' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
enum class Kind { None, Malloc, Calloc, Realloc, Free };
^| const clang::FunctionDecl* FD = CE->getDirectCallee(); | ||
| if (!FD) | ||
| return {}; | ||
| Kind k = llvm::StringSwitch<Kind>(FD->getName()) |
There was a problem hiding this comment.
warning: no header providing "llvm::StringSwitch" is directly included [misc-include-cleaner]
lib/Differentiator/ReverseModeVisitor.cpp:68:
- #include <memory>
+ #include <llvm/ADT/StringSwitch.h>
+ #include <memory>| return AllocCallInfo(k, CE); | ||
| } | ||
|
|
||
| Kind kind() const { return Kind_; } |
There was a problem hiding this comment.
warning: function 'kind' should be marked [[nodiscard]] [modernize-use-nodiscard]
| Kind kind() const { return Kind_; } | |
| [[nodiscard]] Kind kind() const { return Kind_; } |
| } | ||
|
|
||
| Kind kind() const { return Kind_; } | ||
| clang::CallExpr* call() const { return Call_; } |
There was a problem hiding this comment.
warning: function 'call' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::CallExpr* call() const { return Call_; } | |
| [[nodiscard]] clang::CallExpr* call() const { return Call_; } |
| // The number-of-bytes operand that a following memset must zero: | ||
| // malloc(n) -> n, realloc(p, n) -> n. calloc self-zeroes and needs no | ||
| // memset, so it (and free/none) report null here. | ||
| clang::Expr* memsetByteSize() const { |
There was a problem hiding this comment.
warning: function 'memsetByteSize' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::Expr* memsetByteSize() const { | |
| [[nodiscard]] clang::Expr* memsetByteSize() const { |
|
|
||
| private: | ||
| AllocCallInfo(Kind k, clang::CallExpr* c) : Kind_(k), Call_(c) {} | ||
| Kind Kind_ = Kind::None; |
There was a problem hiding this comment.
warning: invalid case style for private member 'Kind_' [readability-identifier-naming]
lib/Differentiator/ReverseModeVisitor.cpp:269:
- Kind kind() const { return Kind_; }
+ Kind kind() const { return m_Kind; }lib/Differentiator/ReverseModeVisitor.cpp:271:
- explicit operator bool() const { return Kind_ != Kind::None; }
+ explicit operator bool() const { return m_Kind != Kind::None; }lib/Differentiator/ReverseModeVisitor.cpp:277:
- switch (Kind_) {
+ switch (m_Kind) {lib/Differentiator/ReverseModeVisitor.cpp:291:
- if (Kind_ != Kind::Realloc || Call_->getNumArgs() == 0)
+ if (m_Kind != Kind::Realloc || Call_->getNumArgs() == 0)lib/Differentiator/ReverseModeVisitor.cpp:301:
- AllocCallInfo(Kind k, clang::CallExpr* c) : Kind_(k), Call_(c) {}
- Kind Kind_ = Kind::None;
+ AllocCallInfo(Kind k, clang::CallExpr* c) : m_Kind(k), Call_(c) {}
+ Kind m_Kind = Kind::None;| private: | ||
| AllocCallInfo(Kind k, clang::CallExpr* c) : Kind_(k), Call_(c) {} | ||
| Kind Kind_ = Kind::None; | ||
| clang::CallExpr* Call_ = nullptr; |
There was a problem hiding this comment.
warning: invalid case style for private member 'Call_' [readability-identifier-naming]
lib/Differentiator/ReverseModeVisitor.cpp:270:
- clang::CallExpr* call() const { return Call_; }
+ clang::CallExpr* call() const { return m_Call; }lib/Differentiator/ReverseModeVisitor.cpp:279:
- return Call_->getArg(0);
+ return m_Call->getArg(0);lib/Differentiator/ReverseModeVisitor.cpp:281:
- return Call_->getArg(1);
+ return m_Call->getArg(1);lib/Differentiator/ReverseModeVisitor.cpp:291:
- if (Kind_ != Kind::Realloc || Call_->getNumArgs() == 0)
+ if (Kind_ != Kind::Realloc || m_Call->getNumArgs() == 0)lib/Differentiator/ReverseModeVisitor.cpp:296:
- Call_->getArg(0)->IgnoreParenCasts());
+ m_Call->getArg(0)->IgnoreParenCasts());lib/Differentiator/ReverseModeVisitor.cpp:301:
- AllocCallInfo(Kind k, clang::CallExpr* c) : Kind_(k), Call_(c) {}
+ AllocCallInfo(Kind k, clang::CallExpr* c) : Kind_(k), m_Call(c) {}| clang::CallExpr* Call_ = nullptr; | |
| clang::CallExpr* m_Call = nullptr; |
e1c50f3 to
ad5fff0
Compare
| return AllocCallInfo(k, CE); | ||
| } | ||
|
|
||
| Kind kind() const { return m_Kind; } |
There was a problem hiding this comment.
warning: function 'kind' should be marked [[nodiscard]] [modernize-use-nodiscard]
| Kind kind() const { return m_Kind; } | |
| [[nodiscard]] Kind kind() const { return m_Kind; } |
| } | ||
|
|
||
| Kind kind() const { return m_Kind; } | ||
| clang::CallExpr* call() const { return m_Call; } |
There was a problem hiding this comment.
warning: function 'call' should be marked [[nodiscard]] [modernize-use-nodiscard]
| clang::CallExpr* call() const { return m_Call; } | |
| [[nodiscard]] clang::CallExpr* call() const { return m_Call; } |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Reverse mode recognised malloc/calloc/realloc by ad-hoc name-string checks in two places: CheckAndBuildCallToMemset re-parsed the RHS for the memset size, and the in-place-realloc test re-parsed it again for the store/restore skip. The two walked the same call independently and are easy to drift apart. Introduce a small AllocCallInfo recognizer that classifies a heap-memory builtin once and owns the per-op invariants -- the memset byte-size operand (malloc arg0 / realloc arg1; calloc self-zeroes) and the in-place realloc check (LHS is realloc's own pointer argument). Both sites now go through it. No functional change: the growing realloc stays valgrind clean and the shrinking case is unchanged.
ad5fff0 to
f467fa8
Compare
1 participant
Reverse mode recognised malloc/calloc/realloc by ad-hoc name-string checks in two places: CheckAndBuildCallToMemset re-parsed the RHS for the memset size, and the in-place-realloc test re-parsed it again for the store/restore skip. The two walked the same call independently and are easy to drift apart.
Introduce a small AllocCallInfo recognizer that classifies a heap-memory builtin once and owns the per-op invariants -- the memset byte-size operand (malloc arg0 / realloc arg1; calloc self-zeroes) and the in-place realloc check (LHS is realloc's own pointer argument). Both sites now go through it. No functional change: the growing realloc stays valgrind clean and the shrinking case is unchanged.