[PATCH] D72741: [MLIR] LLVM dialect: Add llvm.atomicrmw

JF Bastien via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 17 11:11:40 PST 2020


jfb added a comment.

In D72741#1826996 <https://reviews.llvm.org/D72741#1826996>, @flaub wrote:

> I don't think this commit should be about making changes to LLVM IR. I can do that in a followup, but this commit should be in sync with whatever LLVM IR is today. There seems to be consensus with this approach.


Agreed this shouldn't make changes to LLVM IR. However, I'd like to understand whether:

1. a change would occur here first (in this patch, to MLIR), and then in LLVM IR (and who would lead this)
2. in MLIR and LLVM IR at the same time, as a follow-up, and who would lead this
3. never

You're making a new IR, and that's exactly the right time to revisit basic things like this instead of copying LLVM IR. This is so minor, I'm baffled by the pushback. You don't have consensus as evidenced by this discussion, and unwillingness to engage in such a basic question is super worrying. Please don't copy LLVM IR and instead understand why it's designed in a certain manner and what mistakes (however minor) it might contain. I'm not blocking this patch indefinitely, I'm asking you to answer these questions before proceeding. That should take a few minutes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72741/new/

https://reviews.llvm.org/D72741





More information about the llvm-commits mailing list