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

Change offset, len syntax in dma_bd #1137

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

Conversation

makslevental
Copy link
Collaborator

@makslevental makslevental commented Mar 19, 2024

Picked from #1111

@makslevental makslevental changed the title Change offset syntax Change offset, len syntax in dma_bd Mar 19, 2024
Copy link
Contributor

Coverage Report

Created: 2024-03-19 14:30

Click here for information about interpreting this report.

FilenameFunction CoverageLine CoverageRegion CoverageBranch Coverage
IR/AIEDialect.cpp 91.06% 82.55% 85.36% 75.69%
Transforms/AIEObjectFifoStatefulTransform.cpp 100.00% 95.66% 94.44% 89.50%
Totals 93.17% 88.84% 88.55% 80.90%
Generated by llvm-cov -- llvm version 14.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I think GitHub does not let me add comments to lines you didn't change, but line 833 needs to be changed to use the new syntax for dims:

aie.dma_bd(%buf : memref<128xi32>, 0, 128, [<8, 16>, <2, 1>, <8, 2>])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part of the PR, which is only about moving offset and len out into the attr-dict, hasn't been decided on yet (and I haven't rebased either after the first part landed). But I'll try to remember that if it gets green-lit.

@@ -835,13 +863,21 @@ def AIE_DMABDOp: AIE_Op<"dma_bd", []> {
let hasVerifier = 1;

let assemblyFormat = [{
`(` $buffer `:` type($buffer) (`,` $offset^)? (`,` $len^)? (`,` $dimensions^)? `)` attr-dict
`(` $buffer `:` type($buffer) (`,` `dims` `=` $dims^)? `)` attr-dict
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need dims in assemblyFormat? You also add "dims" as an attribute on line 894 below, does that make it part of attr-dict?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All attributes are always in the attribute dictionary (on the C++ side) but the reason I put dims here is because if you put them into attr-dict (i.e., tell the parser to parse them from there) then you have to annotate each size and stride with their type (size: i32, etc) where here you don't need to. You can do some extra work to actually get i32s automatically "inferred" even in the attr-dict (and I tried) but I just don't want to do all of that.

} else {
stride = dims.value()[i].getStride();
size = static_cast<uint16_t>(dims.value()[i].getSize() *
elementWidthIn32bWords);
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears to me from this line we are making the strides be multiples of the memref data type, where previously it was always in 4-byte increments. (Correct me if wrong.)

I'm assuming you all discussed this and agreed to make this change? The reason I bring this up is because we had a lively discussion about this back when I had my original PR for this feature, see here.

I suggested:

So maybe, then, I should issue an error if multi-dim features are used on < i32 arrays. For > i32 arrays, we could have the dimensions be in units of elemental size. So for an i32 array, one unit of stride is 4 bytes, for an i64 arrays, one unit of stride is 8 bytes. This would be consistent with the "length" argument (but I believe the "offset" argument to the dmaBdOp is in bytes ...)

@fifield argued to not do this:

In my opinion the wrap and step should expose the hardware registers directly without trying to do any conversion (e.g. no bytes -> words conversion). This is the cleanest and doesn't require casting to use multi-dimensional BDs in the general case (non-i32 data).

Just want to make sure you are aware of these previous discussions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part of the PR already landed #1111.

It appears to me from this line we are making the strides be multiples of the memref data type, where previously it was always in 4-byte increments. (Correct me if wrong.)

You got it.

I had my original PR for this feature, see #547 (comment).

I wasn't aware such a discussion occurred (would've been nice to know - guess I should've checked...).

Regarding prior opinions/arguments/claims, specifically

It doesn't matter if it's intuitive. That's what the hardware does and at this level of aie dialect we should expose the hardware to the user.

I don't speak for everyone I guess but I think one of the cardinal sins of software is exposing unintuitive interfaces when you have the power to do otherwise. I mean like what's even the point of building an "abstraction" if you're just going to forward arguments/values untouched? Might as well not build it at all and tell people to just use the lower-level interface 🤷.

Too bad I wasn't around when you put up your PR - I would've advocated on your behalf to have it as it is here.

Copy link
Contributor

@andrej andrej Mar 21, 2024

Choose a reason for hiding this comment

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

Okay I will leave it to you whether this warrants further discussion. I think Jeff's argument was that our MLIR dialect is not supposed to be a high level abstraction but instead give easy low level access to the hardware (i.e. allow use of data layout transformations that aren't multiples of the word size without the need for casts). I personally think your change is good though, especially since you also made the offset more coherent.

Why does it show up as a diff here if the PR is already merged though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why does it show up as a diff here if the PR is already merged though?

Because I haven't rebased (so its "base" is some previous commit in main before the merge).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Jeff's argument was that our MLIR dialect is not supposed to be a high level abstraction but instead give easy low level access to the hardware (i.e. allow use of data layout transformations that aren't multiples of the word size without the need for casts).

Yes that's right. I was mostly arguing against casts iirc, and not against intuitive abstractions but for dialect constructs representing the hardware, even if unintuitive. The intuitive interfaces then legalize to the low level (unintuitive) dialect, which is less intended for humans to produce/consume.

@andrej
Copy link
Contributor

andrej commented Mar 20, 2024

Joe pointed me to this, I hope it's cool that I left three quick comments. Looks good

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.

None yet

3 participants