Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make the new operator protected for some IR types. #4670

Merged
merged 2 commits into from
May 27, 2024
Merged

Make the new operator protected for some IR types. #4670

merged 2 commits into from
May 27, 2024

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented May 20, 2024 •
edited
Loading

Addresses some of the points raised in #4612. Note that #4612 is still required. Type pointers can still not be unique because of the source information associated with the type and the clone call.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label May 20, 2024
@fruffy fruffy force-pushed the fruffy/type_getter_private branch from dce0b88 to 53abeeb Compare May 20, 2024 14:18
@vlstill
Copy link
Contributor

vlstill commented May 20, 2024

Addresses some of the points raised in #4612. Note that #4612 is still required. Type pointers can still be unique because of the source information associated with the type and the clone call.

Did you meant to say the type pointers can't be unique?

@fruffy
Copy link
Collaborator Author

fruffy commented May 20, 2024 •
edited
Loading

Addresses some of the points raised in #4612. Note that #4612 is still required. Type pointers can still be unique because of the source information associated with the type and the clone call.

Did you meant to say the type pointers can't be unique?

Yes, it is still possible that IR::LAnd's type evaluates to Type_Unknown because the type of one of the sub expressions may have been cloned previously.

@fruffy fruffy force-pushed the fruffy/type_getter_private branch 3 times, most recently from e79584b to 0032f0d Compare May 20, 2024 15:41
@fruffy fruffy force-pushed the fruffy/type_getter_private branch from 0032f0d to 8af7050 Compare May 20, 2024 15:55
@fruffy fruffy requested review from ChrisDodd and asl May 20, 2024 18:35
ChrisDodd
ChrisDodd approved these changes May 21, 2024
View reviewed changes
backends/bmv2/common/lower.cpp
@@ -81,7 +81,7 @@ const IR::Node *LowerExpressions::postorder(IR::Cast *expression) {
} else if (destType->width_bits() < srcType->width_bits()) {
// explicitly discard un needed bits from src
auto one = new IR::Constant(srcType, 1);
auto shift_value = new IR::Constant(new IR::Type_InfInt(), destType->width_bits());
auto shift_value = new IR::Constant(IR::Type_InfInt::get(), destType->width_bits());
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this (and many other places) could just be rewritten as

auto shift_value = new IR::Constant(destType->width_bits());

the default type for constants created via the ctor that just takes an int is InfInt. I don't think explicitly including the type adds anything -- its more of an obfuscation than a clarification.

ir/base.def
@@ -120,6 +120,7 @@ abstract Type_Base : Type {
class Type_Unknown : Type_Base {
#nodbprint
static Type_Unknown get();
static Type_Unknown get(const Util::SourceInfo &si);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there ever a case where we have an unknown type with a source info? I can't think of how it could occur.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not think so. At least we do not store any source info for an unknown type when parsing. I mostly mirrored what the auto-generated constructors allow, so I do not think at can hurt at least.

asl
asl reviewed May 21, 2024
View reviewed changes
ir/type.cpp Outdated
const Type_Bits *Type_Bits::get(const Util::SourceInfo &si, int sz, bool isSigned) {
auto *result = new IR::Type_Bits(si, sz, isSigned);
if (sz < 0) {
::error(ErrorType::ERR_INVALID, "%1%: Width of type cannot be negative", result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be BUG_CHECK instead? Also, it seems we're creating result here just for some message printing. I think we can just use sz and create result only when all sanity checks pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure why an error was chosen here but the comment

// Return a value that will not cause crashes later on

indicates this was deliberate?

I moved the result allocation down and changed the wording a bit.

@fruffy fruffy enabled auto-merge May 27, 2024 17:59
@fruffy fruffy force-pushed the fruffy/type_getter_private branch from f35ac86 to 6320d18 Compare May 27, 2024 18:06
@fruffy fruffy disabled auto-merge May 27, 2024 18:09
@fruffy fruffy force-pushed the fruffy/type_getter_private branch from 6320d18 to 273155f Compare May 27, 2024 18:49
@fruffy fruffy force-pushed the fruffy/type_getter_private branch from 273155f to cd5028c Compare May 27, 2024 19:19
@fruffy fruffy enabled auto-merge May 27, 2024 20:02
@fruffy fruffy added this pull request to the merge queue May 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 27, 2024
@fruffy fruffy added this pull request to the merge queue May 27, 2024
Merged via the queue into main with commit d504a13 May 27, 2024
16 of 17 checks passed
@fruffy fruffy deleted the fruffy/type_getter_private branch May 27, 2024 21:04
@vlstill vlstill mentioned this pull request May 29, 2024
Closed
@fruffy fruffy added the breaking-change This change may break assumptions of compiler back ends. label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asl asl asl left review comments

@ChrisDodd ChrisDodd ChrisDodd approved these changes

Assignees
No one assigned
Labels
breaking-change This change may break assumptions of compiler back ends. core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fruffy @vlstill @asl @ChrisDodd