[PATCH] D131570: [AArch64][SME] Add utility class for handling SME attributes.

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp:60
+  if (BodyOverridesInterface && Callee.hasStreamingBody()) {
+    bool IsStreaming = hasStreamingInterface() || hasStreamingBody();
+    return IsStreaming ? None : Optional<bool>(true);
----------------
is `hasStreamingInterfaceAndBody()` useful as a method too?


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp:72
+  // Both streaming
+  if ((hasStreamingInterface() || hasStreamingBody()) &&
+      Callee.hasStreamingInterface())
----------------
Again here.


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp:76
+
+  return Callee.hasStreamingInterface() ? true : false;
+}
----------------
Just return the return value?


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h:43-49
+  static SMEAttrs get(unsigned Bitmask) { return SMEAttrs(Bitmask); }
+  static SMEAttrs getNormal() { return SMEAttrs(Normal); }
+  static SMEAttrs getFromFunction(const Function &F) {
+    return getFromAttrList(F.getAttributes());
+  }
+  static SMEAttrs getFromCallBase(const CallBase &CB);
+  static SMEAttrs getFromAttrList(const AttributeList &L);
----------------
Would these not be more natural to create from constructors?


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h:66-67
+
+  Optional<bool> requiresSMChange(const SMEAttrs &Callee,
+                                  bool BodyOverridesInterface = false) const;
+
----------------
This needs documenting, especially `BodyOverridesInterface`.


================
Comment at: llvm/unittests/Target/AArch64/CMakeLists.txt:11
   AArch64Info
+  AsmParser
   CodeGen
----------------
Why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131570