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

[Codegen][GPU] Let integer range optimization narrow GPU computations to i32 #19473

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

krzysz00
Copy link
Contributor

Note: This PR is stacked on top of #19372, and so looks bigger than it is. The relevant changes are in the last commit.

Add an option to -iree-util-optimize-int-arithmetic to have it perform computations in i32 where possible, which is enabled when optimizing arithmetic for GPU codegen. This allows LLVM co correctly conclude that various computations don't need to be done at full 64-bit precision, thus saving registers and instructions. (LLVM has some rewrites for this, but they're, for example, gated on only having one use of the potentially-truncated value, which means that shared math stays in an over-wide data type).

@krzysz00 krzysz00 force-pushed the index-narrowing branch 3 times, most recently from be116ef to ffa5fc4 Compare December 13, 2024 20:21
@krzysz00 krzysz00 marked this pull request as ready for review December 17, 2024 17:28
@krzysz00 krzysz00 force-pushed the index-narrowing branch 2 times, most recently from 01736ba to a367857 Compare January 6, 2025 21:47
ArrayRef<ArrayAttr> castAssumptions = ArrayRef(
static_cast<const ArrayAttr *>(assumptions.data()), assumptions.size());
auto newOp = rewriter.create<Util::AssumeIntOp>(op.getLoc(), newArgs,
castAssumptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you not just do rewriter.create<Util::AssumeIntOp>(op.getLoc(), newArgs, op.getAssumptions());

If not we can either just clone with new operands or add a new builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a builder missing - and no really room to insert one

Copy link
Contributor

Choose a reason for hiding this comment

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

wdym there is no room to add a new builder? Can't we add a builder that take ArrayRef<Value> and ArrayAttr as operands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... Yeah, might not be a bad idea, could add one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants