Remove some trivial leaks in codebase #5012
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,46 +21,46 @@ namespace P4 { | |
| const IR::Node *MoveDeclarations::postorder(IR::P4Action *action) { | ||
| if (findContext<IR::P4Control>() == nullptr) { | ||
| // Else let the parent control get these | ||
| IR::IndexedVector<IR::StatOrDecl> body; | ||
| const auto &m = getMoves(); | ||
| body.insert(body.end(), m.begin(), m.end()); | ||
| body.append(action->body->components); | ||
| action->body = new IR::BlockStatement(action->body->srcInfo, action->body->annotations, | ||
| std::move(body)); | ||
|
There was a problem hiding this comment. Is it actually worth it to use std::move in this case? I understand for the case where you passing along parameters. But here, couldn't the compiler even optimize this? Genuine question. There was a problem hiding this comment. I do not see why compiler would generate something like move ctor here. W/o explicit move it's just plain copy as There was a problem hiding this comment. My thought was that you could see that the local variable is not used after it was passed in here (with some use-def analysis). Then you could elide the copy. But maybe this is too tricky to implement. There was a problem hiding this comment. This is not what C++ standard says. See https://en.cppreference.com/w/cpp/language/copy_elision Without this, copy ctor call is emitted. And it is too late for such optimizations as explicit copies and memory allocation is performed that you cannot remove as they are having side effects. There was a problem hiding this comment. I don't think the C++ compiler is permitted to use move instead of copy. The two constructors are allowed to have observably different behavior, even disregarding the state of moved-from object. As far as I know, all optimizations except for copy elision preserve semantics (at least from single-threaded view). And copy elision can only remove copy, it cannot replace it with There was a problem hiding this comment. In this case the constructors are inline, so the compiler could determine that they don't have observably different behavior, so could be used interchangeably, but it is not required to do so, and it would not surprise me if it did not. There was a problem hiding this comment.
Yeah I was thinking something along that line but I haven't really seen any discussion on this particular matter online. Mostly for my own curiosity because using std::move in this particular scenario seems onerous. |
||
| pop(); | ||
| } | ||
| return action; | ||
| } | ||
|
|
||
| const IR::Node *MoveDeclarations::postorder(IR::P4Control *control) { | ||
| LOG1("Visiting " << control << " toMove " << toMove.size()); | ||
| IR::IndexedVector<IR::Declaration> decls; | ||
| for (auto decl : getMoves()) { | ||
| LOG1("Moved " << decl); | ||
| decls.push_back(decl); | ||
| } | ||
| decls.append(control->controlLocals); | ||
| control->controlLocals = std::move(decls); | ||
| pop(); | ||
| return control; | ||
| } | ||
|
|
||
| const IR::Node *MoveDeclarations::postorder(IR::P4Parser *parser) { | ||
| IR::IndexedVector<IR::Declaration> newStateful; | ||
| newStateful.append(getMoves()); | ||
| newStateful.append(parser->parserLocals); | ||
| parser->parserLocals = std::move(newStateful); | ||
| pop(); | ||
| return parser; | ||
| } | ||
|
|
||
| const IR::Node *MoveDeclarations::postorder(IR::Function *function) { | ||
| IR::IndexedVector<IR::StatOrDecl> body; | ||
| const auto &m = getMoves(); | ||
| body.insert(body.end(), m.begin(), m.end()); | ||
| body.append(function->body->components); | ||
| function->body = new IR::BlockStatement(function->body->srcInfo, function->body->annotations, | ||
| std::move(body)); | ||
| pop(); | ||
| return function; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,17 +36,22 @@ class MoveDeclarations : public Transform { | |
|
|
||
| /// List of lists of declarations to move, one list per | ||
| /// control/parser/action. | ||
| // FIXME: Do we really need IR::Vector here? | ||
| std::vector<IR::Vector<IR::Declaration>> toMove; | ||
|
There was a problem hiding this comment. Do you think There was a problem hiding this comment. Frankly speaking, I do not know. There was a problem hiding this comment. Hmm -- IR::Vector should be 64 bytes larger on a 64 byte machine. Most of that (48 bytes) is for the SourceInfo which is larger than it should be. If the difference is more than that, it implies the C++ compiler is doing something odd (such as using unnecessary redundant pointers in the object for mulitple inheritance). There was a problem hiding this comment.
There was a problem hiding this comment. Yes, INode should be 0 bytes for a reasonable C++ implementation -- everything for the virtual base classes can be in Node's vtable. The fact that it is wasting 24 bytes in every object is annoying. There was a problem hiding this comment. How it will be implemented then? E.g.: class IFoo {
virtual void foo() = 0;
}
class IBar {
virtual void bar() = 0;
}
class IBaz {
virtual void baz() = 0;
}
class IBig : public IFoo, public IBar, public IBaz {
};
class Foo : public virtual IBig {
some fields;
virtual void aaaa();
void foo() override;
void bar() override;
void baz() override;
}
Foo f;
void callBar(IBar *b) {
b->bar();
}
void callBar2(Foo *f) {
f->bar();
}
void callAAAA(Foo *f) {
f->aaaa();
}
callBar(&b);
callBar(f);
callAAAA(f);How we'd make all cases work? There was a problem hiding this comment. Each of the bases has to have its own
All the offsets (for casting etc.) are kept in Footnotes
There was a problem hiding this comment.
It requires laying out the vtables at link time. That requires either a smarter linker, or a common virtual base class for IFoo/IBar/IBaz, in which case you can have a linker section for laying out the vtables of all subclasses of that common virtual base class with even a dumb traditional linker. There was a problem hiding this comment. But you can load a class from a |
||
| void push() { toMove.emplace_back(); } | ||
| void pop() { | ||
| BUG_CHECK(!toMove.empty(), "Empty move stack"); | ||
| toMove.pop_back(); | ||
| } | ||
| const IR::Vector<IR::Declaration> &getMoves() const { | ||
| BUG_CHECK(!toMove.empty(), "Empty move stack"); | ||
| return toMove.back(); | ||
| } | ||
| IR::Vector<IR::Declaration> &getMoves() { | ||
| BUG_CHECK(!toMove.empty(), "Empty move stack"); | ||
| return toMove.back(); | ||
| } | ||
| void addMove(const IR::Declaration *decl) { getMoves().push_back(decl); } | ||
|
|
||
| public: | ||
| explicit MoveDeclarations(bool parsersOnly = false) : parsersOnly(parsersOnly) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be
std::make_shared<CachedLocs>instead?