Skip to content

Recompute the values of CUDA built-in index functions - #1430

Merged
vgvassilev merged 1 commit into
vgvassilev:masterfrom
ovdiiuv:remove-atomics
Jul 7, 2025
Merged

Recompute the values of CUDA built-in index functions#1430
vgvassilev merged 1 commit into
vgvassilev:masterfrom
ovdiiuv:remove-atomics

Conversation

@ovdiiuv

@ovdiiuv ovdiiuv commented Jul 1, 2025

Copy link
Copy Markdown
Collaborator

Before this PR, we used to save the values like threadIdx.x, see exapmle.

__global__ void add_kernel_3(int *out, int *in) {
  int index = threadIdx.x + blockIdx.x * blockDim.x;
  out[index] += in[index];
}

void add_kernel_3_grad(int *out, int *in, int *_d_out, int *_d_in) {
    unsigned int _t1 = blockIdx.x;
    unsigned int _t0 = blockDim.x;
    int _d_index = 0;
    int index0 = threadIdx.x + _t1 * _t0;
    int _t2 = out[index0];
    out[index0] += in[index0];
    {
        out[index0] = _t2;
        int _r_d0 = _d_out[index0];
        atomicAdd(&_d_in[index0], _r_d0);
    }
}

Here we create _t0 to store the blockDim.x, but clearly the function doesn't have any side effects, hence should be recomputed.

@github-actions github-actions Bot left a comment

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.

clang-tidy made some suggestions

Comment thread lib/Differentiator/ReverseModeVisitor.cpp Outdated
@codecov

codecov Bot commented Jul 1, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ovdiiuv
ovdiiuv requested review from kchristin22 and vgvassilev July 1, 2025 16:51
@ovdiiuv
ovdiiuv force-pushed the remove-atomics branch 2 times, most recently from 9263133 to 540098a Compare July 2, 2025 16:12
@github-actions

github-actions Bot commented Jul 2, 2025

Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

isCUDABuiltInIndex(E);
}

bool ReverseModeVisitor::isCUDABuiltInIndex(const Expr* E) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of checking if this is a CUDA Builtin, since effectively these are const variables, can we extend this functionality for all const expressions?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you are right. I took this approach because extracting CallExpr from lets say threadIdx.x is rather ugly, so I guess it deserves to be in a separate function.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree with this being a separate function, but instead of checking the names we can check if it's a const type. In the CUDA case, you can make it to check directly the second subE (OpaqueValueExpr) whether it's a const type. I guess this could work and you wouldn't have to dive that deep as in the case of the CallExpr. But it would be nice if we had a more generic function that checks if the expression is of const type, and one if case there would include this PseudoObjectExpr check-path.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed. I think we would need to look into a solution for a more generic case, which is out of the scope of this PR.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe something like this?

if (const auto *pseudoE = llvm::dyn_cast<PseudoObjectExpr>(B)) {
  if (const auto *opaqueE =
          llvm::dyn_cast<OpaqueValueExpr>(pseudoE->getSemanticExpr(0))) {
      const Expr *innerE = opaqueE->getSourceExpr()->IgnoreImplicit();
      QualType innerT = innerE->getType();
      if (innerT.isConstQualified())
          return true;
  }
 }

@ovdiiuv ovdiiuv Jul 5, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Looks so much better! Thanks:)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This can be a file static function.

@github-actions

github-actions Bot commented Jul 4, 2025

Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@ovdiiuv
ovdiiuv requested a review from kchristin22 July 4, 2025 17:31
@github-actions

github-actions Bot commented Jul 5, 2025

Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions

github-actions Bot commented Jul 5, 2025

Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions

github-actions Bot commented Jul 6, 2025

Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions

github-actions Bot commented Jul 6, 2025

Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev
vgvassilev merged commit b17af42 into vgvassilev:master Jul 7, 2025
88 checks passed
@ovdiiuv
ovdiiuv deleted the remove-atomics branch August 7, 2025 20:10
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