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

Strong types and IndexVec #3122

Merged
merged 13 commits into from
Sep 9, 2024
Merged

Strong types and IndexVec #3122

merged 13 commits into from
Sep 9, 2024

Conversation

braw-lee
Copy link
Contributor

@braw-lee braw-lee commented Aug 5, 2024

No description provided.

@P-E-P P-E-P self-requested a review August 6, 2024 15:29
@braw-lee braw-lee force-pushed the strong_types branch 2 times, most recently from 629f608 to b3696d1 Compare August 7, 2024 09:56
@braw-lee braw-lee marked this pull request as ready for review August 7, 2024 11:50
P-E-P
P-E-P approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

I've got a few questions that I'd like to discuss but overall I love it! Well done!

gcc/rust/checks/errors/borrowck/rust-bir-place.h Show resolved Hide resolved
gcc/rust/typecheck/rust-tyty-variance-analysis.cc Outdated
@@ -585,7 +585,7 @@ FieldVisitorCtx::add_constrints_from_param (ParamType &param, Variance variance)
for (size_t i = type_param_ranges[param_i];
i < type_param_ranges[param_i + 1]; i++)
{
regions.push_back (parent_regions[i]);
regions.push_back (parent_regions[i].value);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we change the type contained into the vector ? This would allow us to remove the .value and keep the type information.

@P-E-P P-E-P requested a review from jdupak August 9, 2024 10:37
jdupak
jdupak approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@jdupak jdupak left a comment

Choose a reason for hiding this comment

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

LGTM
It might be cleaner do have something like IndexVec in rustc to not have to use use the strong types as indices more easily.

gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc Outdated Show resolved Hide resolved
gcc/rust/checks/errors/borrowck/rust-bir-builder-expr-stmt.cc Outdated
@@ -559,7 +559,7 @@ ExprStmtBuilder::visit (HIR::IfExpr &expr)
add_jump (if_block, then_start_block);
add_jump (if_block, final_block);

auto &then_end_bb = ctx.basic_blocks[then_end_block];
auto &then_end_bb = ctx.basic_blocks[then_end_block.value];
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not mind having an implicit conversion for this.

@braw-lee
Copy link
Contributor Author

LGTM It might be cleaner do have something like IndexVec in rustc to not have to use use the strong types as indices more easily.

idk much about IndecVec, will check that to see if we can get something similar here

@braw-lee braw-lee force-pushed the strong_types branch 3 times, most recently from 30c9497 to b4c8e80 Compare August 20, 2024 08:27
@CohenArthur CohenArthur self-requested a review August 23, 2024 10:31
@braw-lee braw-lee changed the title Strong type PlaceId Strong types and IndexVec Aug 23, 2024
@P-E-P P-E-P self-requested a review August 23, 2024 10:31
P-E-P
P-E-P approved these changes Aug 26, 2024
View reviewed changes
Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

Overall it looks good but I wonder if we could implement iterating functions over the BasicBlocks structure. It would simplify a lot of for loops.

gcc/rust/checks/errors/borrowck/rust-bir-dump.cc
@@ -78,13 +82,13 @@ simplify_cfg (Function &func, std::vector<BasicBlockId> &bb_fold_map)
{
stabilized = true;
// BB0 cannot be folded as it is an entry block.
for (size_t i = 1; i < func.basic_blocks.size (); ++i)
for (BasicBlockId i = {1}; i.value < func.basic_blocks.size (); ++i.value)
Copy link
Member

Choose a reason for hiding this comment

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

I don't get it here, why are you creating a basic block here ? From what I can see we could keep a size_t here right ? Since we're iterating over the function basic block, could we even implement an iterator over BasicBlocks ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would argue in the other direction and make bb fold map an index vec.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue with iterator here is starting at one. I think that zero is invalid basic block, but I am not sure (I am on phone right now).

Copy link
Member

Choose a reason for hiding this comment

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

Oh you're right, whilst looking for any iterator blocker in BasicBlocks class I forgot to check right under my nose 😓

jdupak
jdupak reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@jdupak jdupak left a comment

Choose a reason for hiding this comment

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

If we go with strong types it should be all the way, so deconstructing should be rare and hidden.

Otherwise nice job.

@braw-lee
Strong type PlaceId
ba621af 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-dump.cc (renumber_places):
	Use value of PlaceId as index.
	(Dump::visit_place): Likewise.
	(Dump::visit_scope): Likewise.
	(Dump::go): Refill `place_map` with for loop instead of
	using std::iota().
	* checks/errors/borrowck/rust-bir-fact-collector.h (points): Use
	value as index.
	* checks/errors/borrowck/rust-bir-place.h (struct PlaceId):
	PlaceId is now a class holding a uint32_t value. Overloaded
	comparision operators for easier comparision.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Strong type FreeRegion
9bd8a25 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-builder-internal.h: Use
	STATIC_FREE_REGION, use value of FreeRegion for origin.
	* checks/errors/borrowck/rust-bir-builder.h: Use free region
	value.
	* checks/errors/borrowck/rust-bir-dump.cc (Dump::visit_scope):
	Likewise.
	* checks/errors/borrowck/rust-bir-fact-collector.h (points):
	Likewise.
	* checks/errors/borrowck/rust-bir-free-region.h (struct FreeRegion):
	Make FreeRegion a struct.
	* checks/errors/borrowck/rust-bir-place.h: Use FreeRegion
	instead of Origin in PlaceDB.
	* typecheck/rust-tyty-variance-analysis.cc (FieldVisitorCtx::add_constraints_from_region):
	Use value of FreeRegion for origin.
	(FieldVisitorCtx::add_constrints_from_param): Likewise.
	(Term::make_transform): Likewise.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Use FreeRegions inplace of std::vector<FreeRegion>
451e70c 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-builder-internal.h: Use
	FreeRegions instead of making a temporary vector of FreeRegion.
	* checks/errors/borrowck/rust-bir-builder.h: Likewise.
	* checks/errors/borrowck/rust-bir-fact-collector.h (class FactCollector):
	Likewise.
	(points): Likewise.
	* checks/errors/borrowck/rust-bir-free-region.h: Remove obsolete
	set_from() helpers, add push_back().
	* checks/errors/borrowck/rust-bir-place.h: Use FreeRegions
	instead of making a temporary vector of Origin.
	* typecheck/rust-tyty-variance-analysis-private.h: Change type
	of `regions`.
	* typecheck/rust-tyty-variance-analysis.cc (CrateCtx::query_type_regions):
	Use new type.
	(GenericTyPerCrateCtx::query_generic_variance): Likewise.
	(TyVisitorCtx::add_constraints_from_generic_args): Likewise.
	(FieldVisitorCtx::add_constraints_from_region): Likewise.
	(FieldVisitorCtx::add_constrints_from_param): Likewise.
	* typecheck/rust-tyty-variance-analysis.h: Likewise.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Strong type LoanId
5450df4 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-dump.cc (Dump::visit): Use new
	API, i.e get_loan_id() instead of get_loan().
	* checks/errors/borrowck/rust-bir-fact-collector.h (points): Use
	value of LoanId in Polonius facts.
	* checks/errors/borrowck/rust-bir-place.h (struct LoanId):
	LoanId is a struct now.
	* checks/errors/borrowck/rust-bir.h (class AbstractExpr): Use
	new API, as we are getting a LoanId and not a loan itself.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Strong type ScopeId
b738185 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-builder-expr-stmt.cc
	(ExprStmtBuilder::setup_loop): Use value of ScopeId.
	(ExprStmtBuilder::visit): Use continue scope id instead of
	continue basic block id.
	* checks/errors/borrowck/rust-bir-builder-internal.h: Use value
	of ScopeId.
	* checks/errors/borrowck/rust-bir-dump.cc (Dump::go): Use
	ROOT_VALUE instead of hardcoded 0.
	(Dump::visit_scope): Use value of ScopeId.
	* checks/errors/borrowck/rust-bir-place.h (struct ScopeId):
	ScopeId is now a struct.
	(std::numeric_limits::max): Set invalid ScopeId.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Strong type BasicBlockId
a88b635 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-builder-expr-stmt.cc (ExprStmtBuilder::visit):
	Use value of BasicBlockId as index.
	* checks/errors/borrowck/rust-bir-builder-internal.h (struct BuilderContext):
	Likewise.
	* checks/errors/borrowck/rust-bir-builder-lazyboolexpr.h:
	Initialize with ENTRY_BASIC_BLOCK.
	* checks/errors/borrowck/rust-bir-dump.cc (simplify_cfg):
	Use value of BasicBlockId as index.
	(Dump::go): Likewise.
	(Dump::visit): Likewise.
	* checks/errors/borrowck/rust-bir-fact-collector.h (class FactCollector):
	Initialize with ENTRY_BASIC_BLOCK.
	(points): Use value of BasicBlockId as index.
	* checks/errors/borrowck/rust-bir.h (struct BasicBlockId):
	BasicBlockId is a struct now.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Introduce IndexVec
16ccd40 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-place.h (struct Loan):
	Introduce new class `IndexVec` inspired from IndexVec of rust.
	It acts as a wrapper around `std::vector` and lets user specify
	a strong type to use as index.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Used IndexVec for Scopes
78c6e6f 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-place.h:
	Used `IndexVec` with ScopeId as index.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Used IndexVec for Loans
e30124a 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-place.h: Used `IndexVec` with
	ScopeId as index.
	* checks/errors/borrowck/rust-borrow-checker-diagnostics.cc
	(BorrowCheckerDiagnostics::get_loan): Convert Polonius::Loan to
	BIR::Loan, so we can use it as index.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Used IndexVec for BasicBlocks
5b98147 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-builder-expr-stmt.cc
	(ExprStmtBuilder::visit): Use strong type as index and
	remove access to numeric value.
	* checks/errors/borrowck/rust-bir-builder-internal.h
	(struct BuilderContext): Likewise.
	* checks/errors/borrowck/rust-bir-dump.cc (simplify_cfg):
	Likewise.
	(Dump::go): Likewise.
	(Dump::visit): Likewise.
	* checks/errors/borrowck/rust-bir-fact-collector.h
	(class FactCollector): Likewise.
	(points): Likewise.
	* checks/errors/borrowck/rust-bir.h (struct BasicBlockId): Used
	IndexVec for BasicBlocks.
	(struct Function): Likewise.
	* checks/errors/borrowck/rust-borrow-checker-diagnostics.cc
	(BorrowCheckerDiagnostics::get_statement): Change the extracted
	index to strong type.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Used IndexVec for Places
e548ffe 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-place.h: Use strong types as
	index.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Use IndexVec for bb_fold_map
c7d25bf 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-dump.cc (simplify_cfg):
	Used `IndexVec` for bb_fold_map.
	(Dump::go): Use strong type as index instead of value as now we
	are using `IndexVec`.
	(Dump::visit): Likewise.
	* checks/errors/borrowck/rust-bir-dump.h (class Dump): Use
	`IndexVec` for bb_fold_map.
	* checks/errors/borrowck/rust-bir-place.h: Add constructor for
	`IndexVec` that can reserve size.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Used IndexVec for place_map
a5933e7 
gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-dump.cc (Dump::go): Use strong
	type instead of size_t.
	(Dump::visit_place): Likewise.
	(Dump::visit_scope): Likewise.
	* checks/errors/borrowck/rust-bir-dump.h (class Dump): Use
	IndeVec for place_map.

Signed-off-by: Kushal Pal <[email protected]>
P-E-P
P-E-P approved these changes Sep 9, 2024
View reviewed changes
Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

LGTM

@P-E-P P-E-P added this pull request to the merge queue Sep 9, 2024
Merged via the queue into Rust-GCC:master with commit 65b00cc Sep 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdupak jdupak jdupak approved these changes

@P-E-P P-E-P P-E-P approved these changes

@CohenArthur CohenArthur Awaiting requested review from CohenArthur

Assignees
No one assigned
Labels
None yet
Projects
Project Pineapple
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@braw-lee @jdupak @P-E-P @CohenArthur