HIR and new TyTy classes #64

philberty · 2020-12-16T11:12:26Z

Apologies for this big PR but i think it needed to go all together to make sure it all fits. I am trying to follow rustc a bit more now i think its going to solve alot of problems down the road.

HIR classes are based off the AST classes for now eventually this should remove the macro classes once the expand pass is done.

We should also reorder the pipeline a bit, a new name resolution pass should be added and the name resolution pass here should be called name resolution late.

The type resolution converts all types over to a new TyTy module similar to rust this allows for the combination of types the type resolution before was never going to work for cases like:

let mut x;
x = None
x = 1;

there for x is an option or i32.

Or

let mut x = Vec<_>::new()
x.push(1)

therefore x is a vec or i32.

This is going to vastly similify the GIMPLE transformation. My plan so far is that given MIR is a control flow graph it maps fairly close to GIMPLE maybe we have enough info with it to avoid adding MIR down the line.

This is the start of a bigger refactor of the compiler to follow the rustc internals. This introduces a mapping system for. - HirId which maps to any Hir Node within the current crate - LocalDefId any toplevel Hir Node HIR::Item within current crate - NodeId maps any AST node akin to HirId such that they can map back - DefId Cratea and localDefId combination

This adds some extra Flags to ignore unused warnings and no overloaded-virtuals. This helps with compilation error diagnosis.

NodeIds are going to be used for Hir->Ast lookups later on.

This is an IR based off the AST its almost a copy but with NodeMappings on the parent Tree Types. This should ideally have all macro tree's removed during the lowering process.

This is the initial pass to move the AST to HIR. It is very extensible and easy to maintain now.

We can use the NodeId from the AST to generate apropriate mappings for all names and types. Ribs are the scopes for names being instansiated, and reference to defintion tables allows all names to be resolved to NodeId's. Later on NodeIds will map over to HIR ids allowing for type resolution.

Resolution must implement the Gathering specified in the rust-dev guide. We need to be able to handle cases such as: let mut x; x = 1; or let mut x = vec!{} x.push(1) Now the TyTy module has a combine abstract method to allow the combination of types to condense down from their integral parts.

to use the new name and type resolution pass contexts.

SimplyTheOther

This looks great. I had a couple of nitpicks (which I included as inline comments), but nothing too major.

SimplyTheOther · 2020-12-17T12:40:18Z

gcc/rust/ast/rust-pattern.h

  {}

  // Copy constructor with clone
  IdentifierPattern (IdentifierPattern const &other)
    : variable_ident (other.variable_ident), is_ref (other.is_ref),
      is_mut (other.is_mut), locus (other.locus)
  {
-    // fix to prevent null pointer dereference
+    node_id = other.node_id;


Shouldn't this not be required? The implicitly generated copy constructor for Pattern should be able to handle this copy construction of the node_id field.
Unless node_id must be modified by derived classes, avoiding doing things like this by accident would be a good reason to make node_id a private variable in Pattern.

SimplyTheOther · 2020-12-17T12:40:48Z

gcc/rust/ast/rust-pattern.h

@@ -90,6 +92,7 @@ class IdentifierPattern : public Pattern
    is_ref = other.is_ref;
    is_mut = other.is_mut;
    locus = other.locus;
+    node_id = other.node_id;


I don't believe this should be required either, for the same reason.

SimplyTheOther · 2020-12-17T12:45:20Z

gcc/rust/backend/rust-compile-context.h

+  {
+    // insert the builtins
+    auto builtins = resolver->get_builtin_types ();
+    for (auto it = builtins.begin (); it != builtins.end (); it++)


I believe you'd be able to do a range-based for loop here, which would be more concise (though functionally the same). You could have something like this:

for (auto builtin : builtins) { HirId ref; rust_assert (tyctx->lookup_type_by_node_id (builtin->get_node_id (), &ref)); // ... etc }

thanks yeah i should be more consistent i think my brain keeps going between C++11 and old times :)

SimplyTheOther · 2020-12-17T12:59:06Z

gcc/rust/hir/rust-ast-lower.cc

+  bool has_utf8bom = false;
+  bool has_shebang = false;
+
+  for (auto it = astCrate.items.begin (); it != astCrate.items.end (); it++)


I believe you could also use a range-based for loop here for brevity.

SimplyTheOther · 2020-12-17T13:02:50Z

gcc/rust/hir/tree/rust-hir-cond-compilation.h

@@ -0,0 +1,248 @@
+// Copyright (C) 2020 Free Software Foundation, Inc.


This entire file is probably not required at the moment. The current implementation of the cfg attribute expansion just works with the Meta[InsertThingHere] data structures in the AST rather than using these separate configuration ones. As such, nothing would be converted to this HIR as nothing in the AST is stored in this manner.

Thanks i will make an issue to remove these. I basically copied the AST as the beginning point for HIR

SimplyTheOther · 2020-12-17T14:31:00Z

gcc/rust/resolve/rust-ast-resolve.cc

+Resolver::insert_builtin_types (Rib *r)
+{
+  auto builtins = get_builtin_types ();
+  for (auto it = builtins.begin (); it != builtins.end (); it++)


I believe a range-based for loop would be better here for brevity.

SimplyTheOther · 2020-12-17T14:31:58Z

gcc/rust/resolve/rust-ast-resolve.cc

+  resolver->push_new_type_rib (resolver->get_type_scope ().peek ());
+
+  // first gather the top-level namespace names then we drill down
+  for (auto it = crate.items.begin (); it != crate.items.end (); it++)


A range-based for loop would also be more concise here and at line 247 below.

SimplyTheOther · 2020-12-17T14:36:10Z

gcc/rust/hir/tree/rust-hir.h

+};
+
+// A token is a kind of token tree (except delimiter tokens)
+class Token : public TokenTree, public MacroMatch


Assuming that (unexpanded) attributes and macros don't exist in the HIR, tokens shouldn't be required either, I don't think. The AST Token is only used for storing the token data in macro invocations or attributes. Maybe a note of this could be made or something instead of actual deletion at this point, just in case it turns out that tokens are in fact needed.

SimplyTheOther · 2020-12-17T14:40:19Z

gcc/rust/hir/tree/rust-hir-item.h

+  {}
+
+  // Copy constructor uses clone
+  TypeParam (TypeParam const &other)


Again, the modifications to copy constructors that have been made in the AST to prevent null pointer dereferences (having an if statement guard on whether the other variable is null before calling the clone method) should be applied to here, as well as other applicable classes in the HIR.

SimplyTheOther · 2020-12-17T14:41:30Z

gcc/rust/hir/tree/rust-hir-macro.h

+// <http://www.gnu.org/licenses/>.
+
+#ifndef RUST_HIR_MACRO_H
+#define RUST_HIR_MACRO_H


If macros are all expanded in the HIR, then this file should probably not exist. However, just in case they are needed, I would not remove the code here yet.

name resolution and type resolution to simplify the generation of gimple.

philberty requested review from NalaGinrut and SimplyTheOther December 16, 2020 11:12

philberty force-pushed the phil/hir-dev branch from c631b02 to fce071d Compare December 16, 2020 20:21

philberty added 9 commits December 17, 2020 10:17

tweak to rust-system.h includes

3bed27f

Cleanup Makefile for Rust

453510a

This adds some extra Flags to ignore unused warnings and no overloaded-virtuals. This helps with compilation error diagnosis.

Add generated NodeId's to the AST

444cec1

NodeIds are going to be used for Hir->Ast lookups later on.

Add in HIR Tree

25881f3

This is an IR based off the AST its almost a copy but with NodeMappings on the parent Tree Types. This should ideally have all macro tree's removed during the lowering process.

Add AST->HIR lowering pass

6946295

This is the initial pass to move the AST to HIR. It is very extensible and easy to maintain now.

Remove old analysis framework and strip the AST->GIMPLE pass this needs

02796c7

to use the new name and type resolution pass contexts.

philberty force-pushed the phil/hir-dev branch from fce071d to 390cc0b Compare December 17, 2020 12:59

SimplyTheOther approved these changes Dec 17, 2020

View reviewed changes

This is a new HIR -> GIMPLE pass it reuses the mappings from hir,

f2af142

name resolution and type resolution to simplify the generation of gimple.

philberty force-pushed the phil/hir-dev branch from 390cc0b to f2af142 Compare December 17, 2020 16:47

This was referenced Dec 17, 2020

No need for HIR CFGs #68

Closed

Remove macro HIR trees #69

Closed

Remove token trees in HIR #70

Closed

philberty merged commit 6136f0a into master Dec 17, 2020

philberty deleted the phil/hir-dev branch December 17, 2020 17:58

philberty added this to the Core Datastructures milestone Dec 21, 2020

This was linked to issues Jan 3, 2021

Perform basic compilation of a single function #19

Closed

Resolution Design #27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIR and new TyTy classes #64

HIR and new TyTy classes #64

philberty commented Dec 16, 2020 •

edited

Loading

SimplyTheOther left a comment

SimplyTheOther Dec 17, 2020

SimplyTheOther Dec 17, 2020

SimplyTheOther Dec 17, 2020

philberty Dec 17, 2020

SimplyTheOther Dec 17, 2020

SimplyTheOther Dec 17, 2020

philberty Dec 17, 2020

SimplyTheOther Dec 17, 2020

SimplyTheOther Dec 17, 2020

SimplyTheOther Dec 17, 2020

SimplyTheOther Dec 17, 2020

SimplyTheOther Dec 17, 2020

		@@ -0,0 +1,248 @@
		// Copyright (C) 2020 Free Software Foundation, Inc.

HIR and new TyTy classes #64

HIR and new TyTy classes #64

Conversation

philberty commented Dec 16, 2020 • edited Loading

SimplyTheOther left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philberty commented Dec 16, 2020 •

edited

Loading