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.

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

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
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!

@@ -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.

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.

@@ -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
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.

@@ -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 😓

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.

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]>
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]>
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]>
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]>
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]>
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]>
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]>
gcc/rust/ChangeLog:

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

Signed-off-by: Kushal Pal <[email protected]>
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]>
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]>
gcc/rust/ChangeLog:

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

Signed-off-by: Kushal Pal <[email protected]>
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]>
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]>
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
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants