Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Enzyme annotation/attribute plumbing path intended to influence reverse-mode accumulation on GPU targets by disabling atomic updates when a marker attribute is present, and preserves that marker through the NVVM preservation pass.
Changes:
- Teach
PreserveNVVMto convert thellvm.global.annotationsstring"enzyme_elementwise_read"into an LLVM function attribute. - Add a Clang plugin parsed attribute
enzyme_elementwise_readthat lowers toAnnotateAttr("enzyme_elementwise_read"). - Disable reverse-mode atomic accumulation in
DiffeGradientUtils::addToInvertedPtrDiffewhen the function (and nominally a base-pointer argument) carries"enzyme_elementwise_read".
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
enzyme/Enzyme/PreserveNVVM.cpp |
Preserves a new annotation string by translating it into an LLVM function attribute. |
enzyme/Enzyme/DiffeGradientUtils.cpp |
Adds an attribute check that can disable atomic derivative accumulation based on context. |
enzyme/Enzyme/Clang/EnzymeClang.cpp |
Adds Clang-side parsing for enzyme_elementwise_read and lowers it to AnnotateAttr. |
| if (Atomic && elementwiseReadForContext(orig, origptr)) | ||
| Atomic = false; |
There was a problem hiding this comment.
This change alters reverse-mode accumulation behavior by disabling atomics when the new attribute is present, but there’s no lit/IR test exercising the new attribute path (e.g., ensuring PreserveNVVM round-trips the annotation into an LLVM fn attribute and that the generated reverse kernel avoids atomicrmw for that accumulation). Please add a regression test covering both the attribute preservation and the expected removal of atomicrmw in the differentiated output.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
|
@copilot fix this clang format issue "--- enzyme/Enzyme/PreserveNVVM.cpp (original)
|
|
@copilot fix the unit test failure due to the new added ll test "RUN: at line 1: if [ 15 -lt 16 ]; then /opt/homebrew/Cellar/llvm@15/15.0.7/bin/opt < /Users/runner/work/Enzyme/Enzyme/enzyme/test/Enzyme/ReverseMode/elementwise-read.ll --enable-new-pm=0 -load=/Users/runner/work/Enzyme/Enzyme/build/Enzyme/LLVMEnzyme-15.dylib --enzyme-attributor=0 -enzyme-preopt=false -preserve-nvvm -enzyme -enzyme-detect-readthrow=0 -S | FileCheck /Users/runner/work/Enzyme/Enzyme/enzyme/test/Enzyme/ReverseMode/elementwise-read.ll; fi
|
| if (AS == "enzyme_elementwise_read" && Func) { | ||
| Func->addAttribute(AttributeList::FunctionIndex, | ||
| Attribute::get(Func->getContext(), | ||
| "enzyme_elementwise_read")); | ||
| changed = true; | ||
| replacements.push_back(Constant::getNullValue(CAOp->getType())); |
There was a problem hiding this comment.
The PR title/description refer to adding/preserving an "enzyme_noatomic" annotation, but this change implements preservation of "enzyme_elementwise_read" (and there are no occurrences of "enzyme_noatomic" in the code/tests). Please clarify whether the intended attribute name is "enzyme_noatomic" or "enzyme_elementwise_read", and align the implementation + tests + documentation accordingly.
| bool diagAppertainsToDecl(Sema &S, const ParsedAttr &Attr, | ||
| const Decl *D) const override { | ||
| if (isa<FunctionDecl>(D)) | ||
| return true; | ||
| S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type_str) | ||
| << Attr << "functions"; | ||
| return false; |
There was a problem hiding this comment.
This adds support for applying the attribute via the Clang plugin, but the handler only appertains to FunctionDecl. The PR description mentions attaching the annotation to specific pointer arguments; if argument-level marking is required, this should also be permitted on ParmVarDecl (and lowered to an LLVM param attribute or another representation that Enzyme can detect).
| // all additional parallelism in this function is outlined. | ||
| if (backwardsOnlyShadows.find(TmpOrig) != backwardsOnlyShadows.end()) | ||
| Atomic = false; | ||
| if (Atomic && elementwiseReadForContext(orig, origptr)) | ||
| Atomic = false; |
There was a problem hiding this comment.
addToInvertedPtrDiffe already computes TmpOrig = getBaseObject(origptr) just above, but elementwiseReadForContext calls getBaseObject(origptr) again. Since this code is on a hot path, consider reusing the already-computed base object (e.g., pass TmpOrig into the helper or have the helper accept the base Value*) to avoid duplicate value-tracking work.
Preserve and honor enzyme_noatomic on CUDA functions/args
Add support for the "enzyme_noatomic" annotation in PreserveNVVM and
teach the reverse-mode atomic path to disable atomics when either the
function or a base pointer argument carries that attribute. This keeps
CUDA annotations round-tripped into LLVM attributes and applies them
consistently during gradient generation.
Usage: mark a kernel or helper with attribute((annotate("enzyme_noatomic")))
(or attach "enzyme_noatomic" to a specific pointer argument), run
preserve-nvvm before Enzyme, and Enzyme will avoid atomicrmw in the
generated reverse kernel.
the result with and without the ENZYME_NOATOMIC
