Conversation
| Type ptrTy = cp.getDst().getType(); | ||
|
|
||
| auto forOp = scf::ForOp::create(builder, loc, c0, n, c1); | ||
| OpBuilder body(forOp.getBody()->getTerminator()); |
There was a problem hiding this comment.
better to use the provided builder with setInsertionPoint as it can have hooks
There was a problem hiding this comment.
@Pangoraw thanks, I have modified
we will need to include scftocf and cftollvm lowering passes in pipelines
we have covert-polygeist-to-llvm in the downstream, I guess it may could work for it
https://github.com/EnzymeAD/Enzyme-JAX/blob/5db2b680308c38613ed6d5d01ed7d9d9a1353a65/src/enzyme_ad/jax/Passes/ConvertPolygeistToLLVM.cpp#L4573
There was a problem hiding this comment.
ah yes perfect in this case
| return t; | ||
| if (Type t = walk(cp.getSrc())) | ||
| return t; | ||
| return Float64Type::get(cp.getContext()); |
There was a problem hiding this comment.
this is wrong, we should throw an error if things can't be deduced. This is also somewhat hacky and we should use type analysis
There was a problem hiding this comment.
@wsmoses I have fixed it, but only for the case where the inferred element type is a scalar int or float and using upstream MLIR for inferring types. The memcpy ops in RSBench that copy arrays of struct<(f64, f64)> and array<10 x f64> need to handle it recursively, I plan to open a new PR for it.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db457091c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Value byteSz = | ||
| LLVM::ConstantOp::create(builder, loc, len.getType(), | ||
| builder.getIntegerAttr(len.getType(), bytes)); | ||
| Value nInt = LLVM::SDivOp::create(builder, loc, len, byteSz); |
There was a problem hiding this comment.
llvm.intr.memcpy's length is a byte count/size_t, so using signed division makes large i32/i64 lengths with the high bit set become negative; after the index_cast, the generated scf.for can execute zero iterations and leave d_dst unpropagated/unzeroed. Use unsigned division for the element count here.
Useful? React with 👍 / 👎.
| Value gSrc = LLVM::GEPOp::create(builder, loc, ptrTy, elemTy, dSrc, | ||
| ArrayRef<LLVM::GEPArg>{iv}); |
There was a problem hiding this comment.
When a memcpy copies between different LLVM pointer address spaces, dSrc has the source pointer type, but this GEP is forced to return ptrTy from the destination pointer. That emits the reverse source load/store in the wrong address space (or produces invalid LLVM dialect IR) for valid llvm.intr.memcpy calls with differing src/dst address spaces; use the source pointer type for the source shadow GEP.
Useful? React with 👍 / 👎.
4 participants
llvm.intr.memcpy has no registry in reverse AD, which cause "could not compute the adjoint for this operation "llvm.intr.memcpy"".
Reverse rule used here: