RiverDave wrote: > Desing looks good. I can see you have some 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", now 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.
I've Dropped the three helpers from this patch instead of adding tests for them. They have no callers yet, so a test could only re-assert what the verifier already guarantees (host module first, device modules after), which `offload-container.cir` and `invalid-offload-container.cir` cover. They'll come back with their first consumer in the merge/split pass work, where FileCheck over the pass exercises them directly -- think it'll make it easier to review for all of us. https://github.com/llvm/llvm-project/pull/206576 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
