-
Notifications
You must be signed in to change notification settings - Fork 715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GlobalStructInference: Un-nest struct.news in globals when that is helpful #6688
Conversation
|
||
std::unique_ptr<Pass> create() override { | ||
return std::make_unique<FunctionOptimizer>(parent); | ||
const PossibleConstantValues& getConstant() const { |
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.
If you have this return a pointer, then you can avoid the need for a separate isConstant
and you can use it conveniently in if (assignment)
expressions.
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.
Hmm, yeah, that might be a little less code here. But I'm not sure if it would be more clear (as working with pointers is less nice). Also, this is parallel to other existing APIs:
binaryen/src/ir/possible-constant.h
Lines 138 to 154 in 1079a9e
bool isConstant() const { | |
return !std::get_if<None>(&value) && !std::get_if<Many>(&value); | |
} | |
bool isConstantLiteral() const { return std::get_if<Literal>(&value); } | |
bool isConstantGlobal() const { return std::get_if<Name>(&value); } | |
bool isNull() const { | |
return isConstantLiteral() && getConstantLiteral().isNull(); | |
} | |
// Returns the single constant value. | |
Literal getConstantLiteral() const { | |
assert(isConstant()); | |
return std::get<Literal>(value); | |
} |
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.
I see we have divergent styles here. All of the variant-using code I've written uses the pointer approach :)
// See if it matches anything we've seen before. | ||
bool grouped = false; | ||
if (value.isConstant()) { | ||
for (auto& oldValue : values) { |
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.
Would it be worth mapping constants to Value
s to avoid this linear lookup?
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.
I don't think so, since we have a limit on 2 values (so that we can do a single comparison to pick between them).
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.
Ah, right, that makes sense, too.
If we have
then before this PR if we wanted to read the middle field we'd stop, as it is non-constant.
However, we can un-nest it, making it constant:
Now the field is a
global.get
of an immutable global, which is constant. Using thistechnique we can handle anything in a struct field, constant or not. The cost of adding
a global is likely offset by the benefit of being able to refer to it directly, as that opens
up more opportunities later.
Concretely, this replaces the constant values we look for in GSI with a variant over
constants or expressions (we do still want to group constants, as multiple globals
with the same constant field can be treated as a whole). And we note cases where we
need to un-nest, and handle those at the end.