Address review comments

Author: CarolineConcatto

llvm/include/llvm/IR/Intrinsics.td

@@ -52,7 +52,7 @@ def IntrInaccessibleMemOnly : IntrinsicProperty;
 
 
 class IntrinsicMemoryLocation;
-// This should be added in the Target, but once in IntrinsicsAArch64.td
+// These are used to represent Generic Memory Location. 
 def TargetMem0 : IntrinsicMemoryLocation;
 def TargetMem1 : IntrinsicMemoryLocation;
 // IntrInaccessible{Read|Write}MemOnly needs to set Location

llvm/include/llvm/Support/ModRef.h

@@ -73,8 +73,9 @@ enum class IRMemLocation {
 
   /// Helpers to iterate all locations in the MemoryEffectsBase class.
   First = ArgMem,
-  Last = TargetMem1,
   FirstTarget = TargetMem0,
+  // TargetMem IDs must be at the end of the list.
+  Last = TargetMem1,
 };
 
 template <typename LocationEnum> class MemoryEffectsBase {

llvm/lib/IR/AsmWriter.cpp

@@ -5012,7 +5012,7 @@ void AssemblyWriter::writeAllAttributeGroups() {
 
   for (const auto &I : asVec)
     Out << "attributes #" << I.second << " = { "
-    << I.first.getAsString(true) << " }\n";
+        << I.first.getAsString(true) << " }\n";
 }
 
 void AssemblyWriter::printUseListOrder(const Value *V,

llvm/lib/IR/Attributes.cpp

@@ -640,7 +640,10 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
       if (MR == OtherMR)
         continue;
 
-      // Dont want to print Target Location if NoModRef
+      // Dont Print Target Location if MR and target_mems ModRefInfo
+      // are NoModRef.
+      // Printing Inacessiblemem: none implies that target_mems are also
+      // not affected.
       if (ME.isTargetMemLoc(Loc) && (MR == ModRefInfo::NoModRef))
         continue;
 
@@ -660,15 +663,13 @@ std::string Attribute::getAsString(bool InAttrGrp) const {
         break;
       case IRMemLocation::Other:
         llvm_unreachable("This is represented as the default access kind");
-        break;
       case IRMemLocation::TargetMem0:
         OS << "target_mem0: ";
         break;
       case IRMemLocation::TargetMem1:
         OS << "target_mem1: ";
         break;
       }
-
       OS << getModRefStr(MR);
     }
     OS << ")";

llvm/test/TableGen/intrinsic-attrs-fp8.td renamed to llvm/test/TableGen/target-mem-intrinsic-attrs.td

@@ -2,17 +2,17 @@
 
 include "llvm/IR/Intrinsics.td"
 
-def int_aarch64_set_fpmr_2   : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleWriteMemOnly<TargetMem0>]>;
+def int_aarch64_set_target_mem0   : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleWriteMemOnly<TargetMem0>]>;
 
-def int_aarch64_get_za_2   : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<TargetMem1>]>;
+def int_aarch64_get_target_mem1   : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<TargetMem1>]>;
 
-def int_aarch64_get_fpmr_set_za   : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<TargetMem0>, IntrInaccessibleWriteMemOnly<TargetMem1>]>;
+def int_aarch64_get_target_mem0_set_target_mem1   : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [IntrInaccessibleReadMemOnly<TargetMem0>, IntrInaccessibleWriteMemOnly<TargetMem1>]>;
 
 // CHECK:    static constexpr unsigned IntrinsicNameOffsetTable[] = {
 // CHECK-NEXT:  1, // not_intrinsic
-// CHECK-NEXT:  15, // llvm.aarch64.get.fpmr.set.za
-// CHECK-NEXT:  44, // llvm.aarch64.get.za.2
-// CHECK-NEXT:  66, // llvm.aarch64.set.fpmr.2
+// CHECK-NEXT:  15, // llvm.aarch64.get.target.mem0.set.target.mem1
+// CHECK-NEXT:  60, // llvm.aarch64.get.target.mem1
+// CHECK-NEXT:  89, // llvm.aarch64.set.target.mem0
 
 // CHECK:   static AttributeSet getIntrinsicFnAttributeSet(LLVMContext &C, unsigned ID) {
 // CHECK-NEXT:  switch (ID) {
@@ -46,8 +46,3 @@ def int_aarch64_get_fpmr_set_za   : DefaultAttrsIntrinsic<[], [llvm_i64_ty], [In
 // CHECK-NEXT:      Attribute::get(C, Attribute::WillReturn),
 // CHECK-NEXT:      // ArgMem: NoModRef, InaccessibleMem: NoModRef, ErrnoMem: NoModRef, Other: NoModRef, TargetMem0: Mod, TargetMem1: NoModRef
 // CHECK-NEXT:      Attribute::getWithMemoryEffects(C, MemoryEffects::createFromIntValue(512)),
-
-// CHECK:    0 << 1 | 0, // llvm.aarch64.get.fpmr.set.za
-// CHECK-NEXT:    1 << 1 | 0, // llvm.aarch64.get.za.2
-// CHECK-NEXT:    2 << 1 | 0, // llvm.aarch64.set.fpmr.2
-// CHECK-NEXT:};

llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp

@@ -13,6 +13,7 @@
 #include "CodeGenIntrinsics.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/TableGen/Error.h"
@@ -375,13 +376,13 @@ void CodeGenIntrinsic::setProperty(const Record *R) {
   else if (R->getName() == "IntrInaccessibleMemOnly")
     ME &= MemoryEffects::inaccessibleMemOnly();
   else if (R->isSubClassOf("IntrInaccessibleReadMemOnly")) {
-    llvm::IRMemLocation Loc = getLocationTypeAsInt(R, "Loc");
+    llvm::IRMemLocation Loc = getValueAsIRMemLocation(R, "Loc");
     if (ME.onlyAccessTargetMemoryLocation())
       ME = ME.getWithModRef(Loc, ModRefInfo::Ref);
     else
       ME &= MemoryEffects::inaccessibleReadMemOnly(Loc);
   } else if (R->isSubClassOf("IntrInaccessibleWriteMemOnly")) {
-    llvm::IRMemLocation Loc = getLocationTypeAsInt(R, "Loc");
+    llvm::IRMemLocation Loc = getValueAsIRMemLocation(R, "Loc");
     if (ME.onlyAccessTargetMemoryLocation())
       ME = ME.getWithModRef(Loc, ModRefInfo::Mod);
     else
@@ -462,18 +463,22 @@ void CodeGenIntrinsic::setProperty(const Record *R) {
 }
 
 llvm::IRMemLocation
-CodeGenIntrinsic::getLocationTypeAsInt(const Record *R,
-                                       StringRef FieldName) const {
+CodeGenIntrinsic::getValueAsIRMemLocation(const Record *R,
+                                          StringRef FieldName) const {
   const Record *LocRec = R->getValueAsDef(FieldName);
   StringRef Name = LocRec->getName();
-  if (Name == "TargetMem0")
-    return IRMemLocation::TargetMem0;
-  else if (Name == "TargetMem1")
-    return IRMemLocation::TargetMem1;
-  else if (Name == "InaccessibleMem")
-    return llvm::IRMemLocation::InaccessibleMem;
-  else
-    PrintFatalError(R->getLoc(), "unknown IRMemLocation: " + Name);
+
+  IRMemLocation Loc =
+      StringSwitch<IRMemLocation>(Name)
+          .Case("TargetMem0", IRMemLocation::TargetMem0)
+          .Case("TargetMem1", IRMemLocation::TargetMem1)
+          .Case("InaccessibleMem", IRMemLocation::InaccessibleMem)
+          .Default(IRMemLocation::Other); // fallback enum
+
+  if (Loc == IRMemLocation::Other)
+    PrintFatalError(R->getLoc(), "unknown Target IRMemLocation: " + Name);
+
+  return Loc;
 }
 
 bool CodeGenIntrinsic::isParamAPointer(unsigned ParamIdx) const {

llvm/utils/TableGen/Basic/CodeGenIntrinsics.h

@@ -170,8 +170,8 @@ struct CodeGenIntrinsic {
 
   bool isParamImmArg(unsigned ParamIdx) const;
 
-  llvm::IRMemLocation getLocationTypeAsInt(const Record *R,
-                                           StringRef FieldName) const;
+  llvm::IRMemLocation getValueAsIRMemLocation(const Record *R,
+                                              StringRef FieldName) const;
 
   CodeGenIntrinsic(const Record *R, const CodeGenIntrinsicContext &Ctx);
 };