[PATCH] D135592: [IR] Add support for memory attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LG from my end. one nit and one suggestions. I doubt there is anything controversial so this should be OK to land.



================
Comment at: llvm/lib/IR/Attributes.cpp:512
+      OS << getModRefStr(OtherMR);
+    }
+
----------------
nikic wrote:
> jdoerfert wrote:
> > Here and in the examples, did we go back on the `other` front?
> `other` is not available in textual IR, but it is part of the in-memory representation -- we do need *some* way to internally represent everything that does not have a dedicated location kind.
Fair, I got confused by the negative test case below.
That said, can we rename it? Default? Something other than `other`. 
For one, "other" sounds odd, second, it's somewhat tainted now.
Anyway, that should not stop this patch, just a thought.


================
Comment at: llvm/test/Assembler/memory-attribute-errors.ll:1
+; RUN: split-file %s %t
+; RUN: not llvm-as < %t/missing-args.ll 2>&1 | FileCheck %s --check-prefix=MISSING-ARGS
----------------
I didn't know this one, really cool!


================
Comment at: llvm/test/Assembler/memory-attribute.ll:63
+declare void @fn_argmem_read_inaccessiblemem_write()
+    memory(argmem: read, inaccessiblemem: write)
----------------
Add a test that reorders them too. I hope we emit a stable order.


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

https://reviews.llvm.org/D135592