Skip to content
This repository has been archived by the owner on Aug 17, 2022. It is now read-only.

Add system for enabling/disabling optimizations #26

Open
ayazhafiz opened this issue May 1, 2020 · 11 comments
Open

Add system for enabling/disabling optimizations #26

ayazhafiz opened this issue May 1, 2020 · 11 comments
Labels
e:1 Takes a little bit of effort e:2 Takes effort
Milestone

Comments

@ayazhafiz
Copy link
Collaborator

We should add some method for enabling/disabling optimizations as we prepare for the optimization platform. Right now this can be as simple as enabling/disabling whether operations should be evaluated in the partial evaluator.

Related to #7.

@ayazhafiz ayazhafiz added a:lib e:1 Takes a little bit of effort e:2 Takes effort labels May 1, 2020
@ayazhafiz
Copy link
Collaborator Author

I don't know what the best design for this would be. The most straightforward is a map of what optimizations should be enabled:

// This can be packed to 1-bit alignments or we can just use an integer with bitflags idk
struct EvaluationOptions {
  add: bool,
  sub: bool,
  // ...
}

struct PartialEvaluator {
  options: EvaluationOptions
}

fn identity_uneval(l: PEResult, r: PEResult, op: Operator) -> PEResult {
  PEResult::fold_unevaluated_binary(l, r, op)
}

impl PartialEvaluator {
  fn eval_fn(&self, op: Operator) -> Fn(PEResult, PEResult, Operator) {
    match op {
      Plus => {
        if self.options.add { |l, r, op| PEResult::fold_binary(l, r, op) } else { identity_uneval }
      }
    }
  }
}

impl Visitor for PartialEvaluator {
  fn visit_binary_expr(&self, item: Expr) {
    let lhs = self.visit_expr(*item.lhs);
    let rhs = self.visit_expr(*item.rhs);
    let eval_fn = self.eval_fn(expr.op);
    eval_fn(lhs, rhs, item.op)
  }
}

I think a better alternative is to construct the function table when the partial evaluator is first created:

// This can be packed to 1-bit alignments or we can just use an integer with bitflags idk
struct EvaluationOptions {
  add: bool,
  sub: bool,
  // ...
}

trait Evaluator {
  pub fn new(options: EvaluationOptions) -> Self;
}

struct PartialEvaluator {
  eval_fn_table: EvalFnRegistry
}

impl Evaluator for PartialEvaluator {
  fn new(options: EvaluationOptions) -> Self {
    Self {eval_fn_table: EvalFnRegistry::from_options(options)}
  }
}

impl Visitor for PartialEvaluator {
  fn visit_binary_expr(&self, item: Expr) {
    let lhs = self.visit_expr(*item.lhs);
    let rhs = self.visit_expr(*item.rhs);
    let eval_fn = self.eval_fn_table.get_operator(expr.op);
    eval_fn(lhs, rhs, item.op)
  }
}

struct EvalFnRegistry {
  add: Fn(PEResult, PEResult) -> PEResult,
  // ...
}

fn identity_uneval(l: PEResult, r: PEResult, op: Operator) -> PEResult {
  PEResult::fold_unevaluated_binary(l, r, op)
}

impl EvalFnRegistry {
  pub fn from_options(options: EvaluationOptions) -> Self {
    let add = if options.add { |l, r| PEResult::fold_binary(l, r, Plus) } else { identity_uneval } 
  }

  pub fn get_binary_operator(&self, op: Operator) -> Fn(PEResult, PEResult, Operator) {
    match op {
      Plus => self.add,
      // ...
    }
  }
}

@luke-bhan what do you think?

@ayazhafiz ayazhafiz self-assigned this May 1, 2020
@lukebhan
Copy link
Collaborator

lukebhan commented May 2, 2020

I don't think we need two separate structs in addition to partial evaluator for this. I think this can be simplified

@ayazhafiz
Copy link
Collaborator Author

Can you suggest a simplification?

@lukebhan
Copy link
Collaborator

lukebhan commented May 2, 2020

yea, lemme just handle the other pr rq

@lukebhan
Copy link
Collaborator

lukebhan commented May 2, 2020

So wait. If eval_fn is returned false:
does eval_fn(x, y, z) just not run?

@lukebhan
Copy link
Collaborator

lukebhan commented May 2, 2020

also why is EvaluatedOptions a struct and not a hash map

@ayazhafiz
Copy link
Collaborator Author

So wait. If eval_fn is returned false:
does eval_fn(x, y, z) just not run?

Yeah, we just return an unevaluated expression.

also why is EvaluatedOptions a struct and not a hash map

So that we strongly type the kind of options we can have.

@lukebhan
Copy link
Collaborator

lukebhan commented May 2, 2020

I see, the hashmap is a little different in rust then Id expect. I think your solution makes sense then. However, I think were spending a lot of time on unevaluated implementation when all of this will need to be refactored once we start optimizing. Do you think its worthwhile to investigate that first?

@ayazhafiz
Copy link
Collaborator Author

ayazhafiz commented May 2, 2020 via email

@lukebhan
Copy link
Collaborator

lukebhan commented May 2, 2020

Yes, Ill look into it. Also, be safe with the move out.

@ayazhafiz
Copy link
Collaborator Author

ayazhafiz commented May 2, 2020 via email

@ayazhafiz ayazhafiz added this to the Backlog milestone Jun 8, 2020
@ayazhafiz ayazhafiz removed the a:lib label Jun 8, 2020
@ayazhafiz ayazhafiz modified the milestones: Backlog, 0.0.2 Jul 2, 2020
@ayazhafiz ayazhafiz removed their assignment Jul 15, 2020
@ayazhafiz ayazhafiz modified the milestones: 0.0.2, 0.0.3 Sep 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
e:1 Takes a little bit of effort e:2 Takes effort
Projects
None yet
Development

No branches or pull requests

2 participants