https://github.com/koparasy approved this pull request.
Desing looks good. I can see you have 2 deviations from gpu dialect: 1. GPU keeps `verifyOperationAttribute` trivial, just checks the `attr` sits on ModuleOp, and defers structural checks to reference sites. This PR front loads the verification. It is stricter and potentially more costly. 2. You introduce a strict ordering, host is first, gpu dialect does not impose this. Our passes rely on `front() = host`. No strong feelings about this, I would add a comment about it though to document the behavior. 3. gpu.dialect uses a dedicated `gpu.module`; In your PR code lives in nested builtin.module. This is the design you propose. I am ok with this, but before merging I would expect to add a note confirming your design choice. The PR title says "Op", not it is an attribute. Some NITS: The functions `isOffloadContainer` / `getOffloadHostModule` / `getOffloadDeviceModules` have no coverage, and currently are not being called. It is worthwhile to add some unit tests to lock their behavior, since next optimizations will rely on these. https://github.com/llvm/llvm-project/pull/206576 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
