-
Notifications
You must be signed in to change notification settings - Fork 6
WIP: Adding Sycl Support to CYK Calculations #240
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phu, I simply cannot do the review as too many changes are shown that are irrelevant to your work. Could you please a) revert all code style dependent changes and or b) separate them into an independent PR?
src/cpp.cc
Outdated
#include "cpp.hh" | ||
|
||
#include <cmath> | ||
#include <list> | ||
#include <set> | ||
#include <string> | ||
#include <tuple> | ||
#include <utility> | ||
#include <vector> | ||
#include <string> | ||
#include <list> | ||
#include <boost/tokenizer.hpp> | ||
|
||
#include "cpp.hh" | ||
|
||
#include "ast.hh" | ||
#include "cyk.hh" | ||
#include "expr.hh" | ||
#include "expr/new.hh" | ||
#include "fn_def.hh" | ||
#include "grammar.hh" | ||
#include "options.hh" | ||
#include "outside/codegen.hh" | ||
#include "statement.hh" | ||
#include "statement/backtrace_decl.hh" | ||
#include "statement/fn_call.hh" | ||
#include "statement/hash_decl.hh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing. Why did you reorder the imports? Current tests break because of some import issues, is this here the cause? Hard to debug. Could you please revert as many lines as possible to show what really changed?
src/cpp.cc
Outdated
lines.begin(); i != lines.end(); ++i) { | ||
lines.begin(); | ||
i != lines.end(); ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
If you care about code style - and I am a fan of this - we might want to split your PR into one that adds functionality and a second one only addressing code style changes
|
||
#ifndef SRC_CPP_HH_ | ||
#define SRC_CPP_HH_ | ||
|
||
#include <vector> | ||
#include <string> | ||
#include <list> | ||
#include <string> | ||
#include <vector> | ||
|
||
#include "operator.hh" | ||
|
||
#include "printer.hh" | ||
#include "codegen.hh" | ||
|
||
#include "table.hh" | ||
#include "symbol_fwd.hh" | ||
#include "operator.hh" | ||
#include "para_decl_fwd.hh" | ||
#include "printer.hh" | ||
#include "product.hh" | ||
#include "symbol_fwd.hh" | ||
#include "table.hh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same critique applies here as for src/cpp.cc
. This PR is overly hard to understand as the many thousand lines of code that changed just because of code-style are very hard to differentiate from conceptional changes :-/
new Type::String, "test_value"); | ||
|
||
fn_cyk->stmts.push_back( | ||
new Statement::SYCL_Buffer_Decl(new Type::Int, dimension, value, value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are going to use such a statement, don't you have to include according cycl/cycl.hpp header files?! The currently failing tests point to exactly these headers being absent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We furthermore might want to think about a user switch/flag to decide if generated code shall contain sycl "extensions" or not?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is the plan for this week, implementing headers and the proper loop
This is the pull request that should be used to discuss the sycl specific changes. It is currently only a draft.