Skip to content

[Alloc] Fixed alignment for shared memory allocation#3854

Merged
Jokeren merged 3 commits intotriton-lang:mainfrom
ROCm:alloc-alignment-fix
May 13, 2024
Merged

[Alloc] Fixed alignment for shared memory allocation#3854
Jokeren merged 3 commits intotriton-lang:mainfrom
ROCm:alloc-alignment-fix

Conversation

@sjw36
Copy link
Copy Markdown
Contributor

@sjw36 sjw36 commented May 7, 2024

  • Increment buffers just past max interference buffer

* Increment buffers just past max interference buffer
@sjw36 sjw36 requested review from Jokeren and ptillet as code owners May 7, 2024 20:53
@sjw36
Copy link
Copy Markdown
Contributor Author

sjw36 commented May 7, 2024

@ThomasRaoux @Jokeren , this should be just the alignment fix.

@sjw36
Copy link
Copy Markdown
Contributor Author

sjw36 commented May 7, 2024

I did remove the bufferStart since it seemed unnecessary.

Comment thread lib/Analysis/Allocation.cpp Outdated
std::min(range.end(), xRange.end())}});
size_t offset = size;
if (size_t diff = offset % buffer->alignment)
offset += buffer->alignment - diff;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the difference between this and the previous calculation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

        if (size_t diff = newOffset % x->alignment) {
          // fix alignment
          newOffset += x->alignment - diff;
        }

I'm fine with the change. It's just that:

  1. I'm curious about the differences
  2. And maybe you could make this as a function as it's being used multiple times (here and below).

Copy link
Copy Markdown
Contributor Author

@sjw36 sjw36 May 7, 2024

Choose a reason for hiding this comment

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

It gets the same value, just a different way to compute. I just thought this was more clear but the alternative is a single unconditional line. I'll make it a BufferT member function.

Comment thread lib/Analysis/Allocation.cpp Outdated
@Jokeren
Copy link
Copy Markdown
Contributor

Jokeren commented May 7, 2024

@sjw36 Thank you Simon!

Comment thread include/triton/Analysis/Allocation.h
@Jokeren Jokeren enabled auto-merge (squash) May 10, 2024 18:42
@sjw36 sjw36 requested a review from antiagainst May 10, 2024 22:37
@sjw36
Copy link
Copy Markdown
Contributor Author

sjw36 commented May 10, 2024

@antiagainst your request should be resolved. Please verify

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