Skip to content

Use exactness to optimize GUFA cone depth #7538

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

Closed
wants to merge 12 commits into from
Closed

Conversation

tlively
Copy link
Member

@tlively tlively commented Apr 22, 2025

Exact references are known to point to exactly their heap type and not
one of its subtypes. GUFA already analyzed exactness via the more
general "cone" types that include an allowed subtyping depth. Exact
types are equivalent to cone types of depth zero. Let GUFA take
advantage of exactness information by normalizing cone depths to 0
whenever the cone type is exact.

@tlively tlively requested a review from kripken April 22, 2025 00:36
;; We should be able to infer that this cast will not succeed and insert an
;; unreachable after it.
(ref.cast (ref $sub)
(call $get-exact)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why doesn't this work without exact types? GUFA's depth zero cone should have had enough information already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the sources are imports, not allocations, so without the new changes, GUFA doesn't know that it can use 0-depth cones for those imports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, thanks!

Base automatically changed from gufa-mixin-null-exact to main April 22, 2025 15:35
@@ -2135,6 +2135,9 @@ struct Flower {
// For a non-full cone, we also reduce the depth as much as possible, so it is
// equal to the maximum depth of an existing subtype.
Index getNormalizedConeDepth(Type type, Index depth) {
if (type.isExact()) {
return 0;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be a better place:

diff --git a/src/ir/possible-contents.h b/src/ir/possible-contents.h
index 538b23d4d..ab5b53436 100644
--- a/src/ir/possible-contents.h
+++ b/src/ir/possible-contents.h
@@ -164,8 +164,8 @@ public:
     assert(type != Type::none);
 
     if (type.isRef()) {
-      // For a reference, subtyping matters.
-      return fullConeType(type);
+      // For a reference, subtyping matters, unless it is exact.
+      return type.isExact() ? exactType(type) : fullConeType(type);
     }
 
     if (type == Type::unreachable) {

That applies exactness on creation. I think it should be enough here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like fullConeType is also used directly in several places. WDYT about having this check in fullConeType so it's harder to inadvertently skip?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. Yeah, that seems safest.

@tlively tlively requested a review from kripken April 22, 2025 18:52
bool hasFullCone() const { return getCone().depth == FullDepth; }
bool hasFullCone() const {
return getCone().depth == (getType().isExact() ? 0 : FullDepth);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to change? It seems odd to consider a code of depth 0 as "full".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otherwise a bunch of assertions expecting full cone types started failing. I think it makes sense to make cone creation and inspection consistent in treating depth-0 cones for exact types as "full," so I think this is a better solution than going and changing those assertions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to understand this better - which assertions were those?

Copy link
Member Author

@tlively tlively Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion at the top of PossibleContents::intersect.

// This does not yet handle all possible content.
assert(other.isFullConeType() || other.isLiteral() || other.isNone());

Looking deeper into PossibleContents::intersect and the other methods it calls, it's a little difficult to tell whether they do the right thing with exact types. I propose 1) moving forward with this change as-is to see if further fuzzing with exact types turns up any issues and 2) adding more possible-contents gtests covering exact types in a follow-up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that assert looks suspicious to me... I'd rather investigate that first, but I don't feel strongly.

If you want to land this quickly to unblock other stuff, how about doing the reverse: modify the assert to allow either a full cone or an exact type, so we don't need to modify hasFullCone? I can also take a look at the PossibleContents code if you want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No rush to land this. I can reorder things to avoid getting blocked. I do think it would be weird if a value created by fullConeType did not satisfy hasFullCone or isFullCone, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should rename fullConeType probably. Really what it does now is create a cone based on the type. Just coneType might be enough.

tlively added 12 commits April 25, 2025 18:31
Exact references are known to point to exactly their heap type and not
one of its subtypes. GUFA already analyzed exactness via the more
general "cone" types that include an allowed subtyping depth. Exact
types are equivalent to cone types of depth zero. Let GUFA take
advantage of exactness information by normalizing cone depths to 0
whenever the cone type is exact.
@tlively
Copy link
Member Author

tlively commented May 13, 2025

Closing in favor of #7582

@tlively tlively closed this May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants