For kernel SPIR-V, we are lowering memref to bare pointers, so reinterpret can be lowered to pointer, adjusted by offset value.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The logic seems fine to me, just some stylistic issues.
@antiagainst, can you also take a look?
mlir/lib/Conversion/MemRefToSPIRV/MemRefToSPIRV.cpp | ||
---|---|---|
260 | nit: add final | |
265–266 | For consistency with the other code in this file, this function should be defined outside of the class definition. | |
272–273 | nit: Use llvm::formatv for the diag message: rewriter.notifyMatchFailure(op, llvm::formatv(...)). You can find examples in other spirv patterns, so this is mostly for consistency with that code. | |
276 | nit: auto doesn't improve readability here IMO, consider spelling out the type whenever it's not obvious based on the RHS | |
279–280 | nit: Use converter->convertType<spirv::PointerType>(...) | |
282–283 | also here | |
298 | nit: Location loc |
LGTM. Just a few more nits.
mlir/lib/Conversion/MemRefToSPIRV/MemRefToSPIRV.cpp | ||
---|---|---|
277 | This assert is not necessary? The converter pretty much always exists there. | |
296 | "failed to convert index type" to be clear. | |
311 | Nit: I'd just do auto acOp = rewriter.create<spirv::InBoundsPtrAccessChainOp>(loc, src, offsetValue, std::nullopt); rewriter.replaceOp(op, acOp.getResult()); to make it indent nicer. |
nit: add final