jyknight wrote: > The tests posted in the review work from that point on, except [...]
Sure, I agree that the tests which have been added pass. But the tests do not currently test everything necessary for this feature to be complete. What I see here is a series of PRs each adding some part of a full implementation of this feature, and some corresponding tests for those pieces. But, at the moment it looks like just a bunch of individual pieces of work, not a whole picture of where this is going. I can't tell if missing/broken functionality is missing because you hadn't thought of it yet, or because this PR series does not yet contain all the work you know you will need to eventually do, or what. If breaking work up into a series of PRs, the series needs to tell an understandable story. E.g., it could be something like this: - New IR allowed in the verifier. It will not yet compile, but bitcode/textual IR creation/reading/writing works. - AtomicExpandPass handles vector types correctly. Now the IR-level transform works; backend target lowering still broken. - Generic SelectionDAG legalization support added. (prerequisite work for next PRs) - X86 backend support. Now, `store atomic <N x T>` and `load atomic <N x T>` should correctly compile on x86/x86_64 platforms (for all N and T), when using SelectionDAG. - GlobalISel - Other targets - Now, the new operations are fully functional. (I'm not saying it has to be exactly _that_ split, it's just an example.) https://github.com/llvm/llvm-project/pull/120385 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
