BUG-2199: Fix for the FSRT Multiple Definitions for Same Variable Bug #47

swathiatgit · 2024-07-19T19:52:47Z

FSRT should be in “static single assignment form”, i.e. there should be exactly one definition of each variable, as this simplifies later analysis. This PR fixes this bug, and ensures that every variable has one definition.

More on the bug: Take this code example below:

function foo(a) {
  let b = a - 20;
  let c = b + 1;
  b = 10;
  return b * 10;
}

The above function currently gives this IR:

Variables:
%0: return value
%1: local definition of b
%2: local definition of c
%3: local definition of b
%4: local definition of b
%5: global ref to a
%6: temporary
%7: global ref to b
%8: temporary
%9: global ref to c
%10: temporary
bb0:
    %6 = %5 - 20
    %7 = %6 
    %8 = %7 + 1
    %9 = %8
    %7 = 10 // second assignment of %7
    _ = 10
    %10 = %7 * 10
    %0 = %10
    return

Note that in the current output of our IR that every “use” of the variable %7 only needs to consider the definition of “%7 = 10”. Thus, we can replace the assignment “%7 = 10” with “%11 = 10” and all following uses of “%7” with the new variable “%11”.

!

atlassian-cla-bot · 2024-07-19T19:52:50Z

Thank you for your submission! Like many open source projects, we ask that you sign our CLA (Contributor License Agreement) before we can accept your contribution.
If your email is listed below, please ensure that you sign the CLA with the same email address.

The following users still need to sign our CLA:
❌snarayan2@dk9nqx9wdq.office.atlassian.com

_{Already signed the CLA? To re-check, try refreshing the page.}

gersbach · 2024-07-22T16:21:02Z

crates/forge_analyzer/src/definitions.rs

@@ -1464,7 +1467,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
                                    let id = ident.to_id();
                                    let new_def =
                                        self.res.get_or_insert_sym(id.clone(), self.module);
-                                    let var_id = self.body.get_or_insert_global(new_def);
+                                    let var_id = self.body.insert_global(new_def);


@jwong101 @jwong33 i know we haven't been strict about LocalDef vs GlobalRef previously... do we want to start?

Yeah, we shouldn't be creating new global variables/var ids, since this will create two separate variables:

const b = 3; const a = { b };

%b = 3 %a.b = %newb

gersbach · 2024-07-22T16:21:25Z

Very clean code 🙂

jwong101

The main problem with doing this during lowering is that you need to keep track of which uses of a symbol correspond to the new definition that you created. Right now it's implicitly the last one, which doesn't work for control flow across basic blocks.

Also, new uses of an identifier shouldn't create another VarId, rather it should refer to the last VarId that was assigned.

Regardless, nice work so far! Let me know if you have any questions.

jwong101 · 2024-07-22T17:12:20Z

crates/forge_analyzer/src/definitions.rs

@@ -1464,7 +1467,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
                                    let id = ident.to_id();
                                    let new_def =
                                        self.res.get_or_insert_sym(id.clone(), self.module);
-                                    let var_id = self.body.get_or_insert_global(new_def);
+                                    let var_id = self.body.insert_global(new_def);


Yeah, we shouldn't be creating new global variables/var ids, since this will create two separate variables:

const b = 3; const a = { b };

%b = 3 %a.b = %newb

gersbach · 2024-07-22T17:33:55Z

The main problem with doing this during lowering is that you need to keep track of which uses of a symbol correspond to the new definition that you created. Right now it's implicitly the last one, which doesn't work for control flow across basic blocks.

Also, new uses of an identifier shouldn't create another VarId, rather it should refer to the last VarId that was assigned.

Regardless, nice work so far! Let me know if you have any questions.

@jwong101, I thought this bug was for within basic blocks though? Shouldn't we wait until the dominator tree is merged in and we can track between basic blocks then?

swathiatgit · 2024-07-22T18:43:21Z

Thank you for the feedback @jwong101, taking a look at this right now. I believe the new uses of an identifier case should be fairly simple to resolve - I'm investigating it now. Regarding the issue with lowering and needing to track which uses of a symbol correspond to the new definition created, let me ensure that it's explicitly defined and not implicitly.

Ideally, if it's explicitly defined, I think that should translate fairly smoothly to incorporating the dominator tree work as well. I'll touch base with you and @gersbach regarding this soon when I've done more work to research it. Thank you!

jwong101

Added comments where extraneous var ids are created.

While you can go with this approach, it will make the renaming trickier in later stages. Mainly because you now have to do another lookup to check the "true" alias of a var ID on every use to do the correction. I.e.:

// mapping
// %0 -> a
// %1 -> b
// %2 -> a
// %3 -> c
// %4 -> d

let a = 1; // %0 = 1
if (true) {
  let b = a // %1 = %0
  a = 3; // %2 = 3
  let c = a; // %3 = %2
  return;
}
let d = a; // %4 = %2 ((assuming we visit the if statement bb first)

While you could solve this by checking the mapping, it's a lot easier to just treat the raw var IDs as the potential aliases.

Though one thing you could do in the lowerering code is treating global assignments differently(basically variables that aren't defined in the function body). Those can't be in SSA form, since the assignment semantics are different. (they should have a separate IR statement tbh)

jwong101 · 2024-07-24T17:50:47Z

crates/forge_analyzer/src/definitions.rs

@@ -1394,7 +1397,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
            warn!("unknown symbol: {}", id.0);
            return Literal::Undef.into();
        };
-        let var = self.body.get_or_insert_global(def);
+        let var = self.body.insert_global(def);


This will create duplicate var IDs on each use of ident, which will make coalescing them trickier when handling the multi basic block case.

jwong101 · 2024-07-24T17:55:11Z

crates/forge_analyzer/src/definitions.rs

@@ -1192,7 +1195,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
                    .iter()
                    .find(|(name, defid)| name == "constructor")
                {
-                    let var = self.body.get_or_insert_global(*def_constructor);
+                    let var = self.body.insert_global(*def_constructor);


Same here, uses shouldn't create new defs.

jwong101 · 2024-07-24T17:55:40Z

crates/forge_analyzer/src/definitions.rs

@@ -1206,7 +1209,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
                            .iter()
                            .find(|(name, defid)| name == &ident.sym)
                        {
-                            let var = self.body.get_or_insert_global(*def_constructor);
+                            let var = self.body.insert_global(*def_constructor);


Same here, this is a use.

jwong101 · 2024-07-24T17:56:14Z

crates/forge_analyzer/src/definitions.rs

@@ -1351,7 +1354,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
                }
                Expr::Ident(ident) => {
                    let defid = self.res.sym_to_id(ident.to_id(), self.module);
-                    let varid = self.body.get_or_insert_global(defid.unwrap());
+                    let varid = self.body.insert_global(defid.unwrap());


Use not a def

jwong101 · 2024-07-24T17:57:14Z

crates/forge_analyzer/src/definitions.rs

@@ -1414,7 +1417,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
                    warn!("unknown symbol: {}", id.0);
                    return Literal::Undef.into();
                };
-                let var = self.body.get_or_insert_global(def);
+                let var = self.body.insert_global(def);


use not a def

jwong101 · 2024-07-24T17:57:22Z

crates/forge_analyzer/src/definitions.rs

@@ -1424,7 +1427,7 @@ impl<'cx> FunctionAnalyzer<'cx> {
                    warn!("unknown symbol: {}", ns.0);
                    return Literal::Undef.into();
                };
-                let var = self.body.get_or_insert_global(def);
+                let var = self.body.insert_global(def);


use not a def

Swathi Narayan added 4 commits July 14, 2024 13:44

SSA bug resolved for literal assignments

607f2f7

Fixed variable re-reference bug, checking ordering

b56b7b0

Efficient solution implemented! All tests pass

bdb4bbb

Cleaned up code, ready to merge

7280c35

!

Added documentation, ready to merge

78df859

gersbach reviewed Jul 22, 2024

View reviewed changes

jwong101 requested changes Jul 22, 2024

View reviewed changes

New identifiers for object as field bug fix

33d2a07

jwong101 requested changes Jul 24, 2024

View reviewed changes

swathiatgit closed this Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG-2199: Fix for the FSRT Multiple Definitions for Same Variable Bug #47

BUG-2199: Fix for the FSRT Multiple Definitions for Same Variable Bug #47

swathiatgit commented Jul 19, 2024

atlassian-cla-bot bot commented Jul 19, 2024

gersbach Jul 22, 2024

jwong101 Jul 22, 2024

gersbach commented Jul 22, 2024

jwong101 left a comment •

edited

Loading

jwong101 Jul 22, 2024

gersbach commented Jul 22, 2024

swathiatgit commented Jul 22, 2024

jwong101 left a comment

jwong101 Jul 24, 2024

jwong101 Jul 24, 2024

jwong101 Jul 24, 2024

jwong101 Jul 24, 2024

jwong101 Jul 24, 2024

jwong101 Jul 24, 2024

BUG-2199: Fix for the FSRT Multiple Definitions for Same Variable Bug #47

BUG-2199: Fix for the FSRT Multiple Definitions for Same Variable Bug #47

Conversation

swathiatgit commented Jul 19, 2024

atlassian-cla-bot bot commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gersbach commented Jul 22, 2024

jwong101 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gersbach commented Jul 22, 2024

swathiatgit commented Jul 22, 2024

jwong101 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

jwong101 left a comment •

edited

Loading