-
Notifications
You must be signed in to change notification settings - Fork 189
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
Linear Algebra API & Parser #364
base: master
Are you sure you want to change the base?
Conversation
…es LinalgExpr and LinalgBase
…st Vector constructor
…dd new compound test
Fixes assertion failure in LinalgBase::operator= (compiled in debug mode)
Datatype getDataType() const; | ||
int getOrder() const; | ||
bool isColVector() const; | ||
void setColVector(bool) const; |
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.
What is the intended use case for this method? I don't see this invoked anywhere.
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.
@stephenchouca This setColVector()
function is for when the user defines a vector as row (or col) in the constructor, but then later realizes the colVector
flag needs to be changed. It's not used anywhere but is provided as part of the user LA API
|
||
Datatype getDataType() const; | ||
int getOrder() const; | ||
bool isColVector() const; |
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.
What should this return for scalars or matrices?
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 returns false for vectors and matrices always. Do you have a better suggestion of how to do this?
public: | ||
explicit Matrix(std::string name); | ||
|
||
Matrix(std::string name, size_t dim1, size_t dim2); |
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.
dim1
and dim2
should probably just be named rows
and cols
?
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.
Will change this
} | ||
|
||
// Read method | ||
CType at(int coord_x, int coord_y); |
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.
Not sure why coord_x
and coord_y
are used here when i
and j
are used for operator()
. I think it'd make more sense to use one or the other consistently.
IndexStmt indexAssignment; | ||
|
||
int idxcount = 0; | ||
std::vector<std::string> indexVarNameList = {"i","j","k","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z"}; |
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 line seems a bit long. Whenever possible, lines should be kept to 80 characters max (here and in other parts of code base).
std::vector<IndexVar> getUniqueIndices(size_t order); | ||
|
||
public: | ||
LinalgBase(std::string name, Type tensorType, Datatype dtype, std::vector<int> dims, Format format, bool isColVec = false); |
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 does the constructor need to take both tensorType
and dims
as arguments? Doesn't tensorType
have a shape
member that should always be the same as dims
?
namespace taco { | ||
|
||
|
||
struct LinalgVarNode : public LinalgExprNode { |
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 and next few classes probably don't need to be indented?
|
||
class LinalgBase; | ||
|
||
class LinalgRewriter : public util::Uncopyable { |
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.
The name "rewriter" is somewhat confusing since it suggests that rewrite
returns a new LinalgStmt
or LinalgExpr
as input (similar to how IndexNotationRewriter
takes IndexExpr
as input and returns IndexExpr
as output). Maybe something like LinalgLowerer
would be less confusing?
return result; | ||
} | ||
|
||
IndexExpr LinalgBase::rewrite(LinalgExpr linalg, vector<IndexVar> indices) { |
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.
Should probably also consider renaming these methods? (See comment regarding LinalgRewriter
)
This adds a new front-end for manipulating 1D/2D tensors (vectors, matrices) with linear algebra notation. Matrix/Vector multiplication, transpose and element-wise ops are supported.
Using the "-linalg" flag with ./bin/taco turns on parsing of linear algebra expressions. Shapes of the linear algebra variables can be (optionally) specified via the "-k=:" flag. If not specified, variable names starting with capital letters are assumed to be matrices.
Example usage:
./bin/taco "A=B*C + transpose(x * transpose(y))" -linalg
Besides the parser, an API that provides Matrix and Vector classes has been added. Tests demonstrating its use are in tests/tests-linalg.cpp.
Slides from talk given at TACO Weekly (1/6/2021) here:
https://docs.google.com/presentation/d/1zX9mUt9iqvwK6vVuakI882s7mc8-XNbUmqXRIw_HI5U/edit?usp=sharing