Skip to content
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

Lower MemRef GetGlobal and write data to json files #7301

Merged
merged 16 commits into from
Dec 5, 2024

Conversation

jiahanxie353
Copy link
Contributor

@jiahanxie353 jiahanxie353 commented Jul 10, 2024

This patch lowers memref::GetGlobalOp and write data that it carries to corresponding json files, so that they can be used later by Calyx. Json data will be written to the same directory as the source file. Since there might be situations where users don't wish to do so, writing to json will only be carried out if users specify --lower-scf-to-calyx="write-json=true".

@jiahanxie353 jiahanxie353 marked this pull request as ready for review July 10, 2024 19:59
@rachitnigam rachitnigam added the Calyx The Calyx dialect label Jul 29, 2024
@jiahanxie353 jiahanxie353 force-pushed the lower-global branch 2 times, most recently from 0e020a7 to e8f9e95 Compare November 17, 2024 02:14
@jiahanxie353
Copy link
Contributor Author

I have switched to using llvm::json; not sure how to test the output flushed to a json file though..

@jiahanxie353
Copy link
Contributor Author

Test case included! Ready for another round of review!

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
test/Conversion/SCFToCalyx/write_memory.mlir Show resolved Hide resolved
test/Conversion/SCFToCalyx/write_memory.mlir Show resolved Hide resolved
include/circt/Dialect/Calyx/CalyxLoweringUtils.h Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
@jiahanxie353
Copy link
Contributor Author

fixed the signless integer problem!

Copy link
Member

@cgyurgyik cgyurgyik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! A few smaller style comments, but overall looks great.

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
@jiahanxie353
Copy link
Contributor Author

Made changes based on the feedback, thank you @cgyurgyik !
And now including all floating point type, signless integer type, and signed integer type in testing to test comprehensively.

Copy link
Member

@cgyurgyik cgyurgyik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A final few nits, please address them as you see fit. LGTM once tests pass. Nice job!

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
lib/Conversion/SCFToCalyx/SCFToCalyx.cpp Outdated Show resolved Hide resolved
llvm::json::Array result;
result.reserve(std::max(static_cast<int>(shape.size()), 1));

bool isSignlessOrUnsigned = memtype.getElementType().isSignlessInteger() ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: it seems that every place you use this its prefixed by !. Maybe just do

Type elemType = memtype.getElementType();
bool isSigned = !elemType.isSignlessInteger() && !elemType.isUnsignedInteger();

(Although isn't that just the same as isSignedInteger()?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good!

Although isn't that just the same as isSignedInteger()?

isSigned should be true when isSignedIntegerOrFloat, but there is no such built-in check; only isSignlessOrFloat is present, but that's not what we want.
Or we can use isSignedInteger || isFloat, but there is again no built-in isFloat check, only those specific checks like isFloat8E3M4().
So I have to use the negation logic..

@jiahanxie353
Copy link
Contributor Author

Planning to squash and merge once the CI passes. Thank you for the review!

@jiahanxie353 jiahanxie353 self-assigned this Dec 5, 2024
@jiahanxie353 jiahanxie353 merged commit baba42c into llvm:main Dec 5, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Calyx The Calyx dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants