[PATCH] D76175: [Attributor][NFC] Typos in doc

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis created this revision.
baziotis added reviewers: jdoerfert, uenoku, sstefan1.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.
baziotis edited the summary of this revision.
baziotis added a comment.

Today I started reading through the whole Attributor, or at least its core parts, because I think otherwise I won't
have a good enough understanding to proceed. This is the start of a series of small patches that will mostly be NFC
improvements to docs with which I hope I'll have the opportunity to ask questions about different decisions in the Attributor.

Starting off, I tried multiple times to read `IRPosition` and in my humble opinion, it seems //way// too complicated.
Specifically the `KindOrArgNo`. This coupling of basically 2 kinds of info in one seems unnecessary. Plus, `IRP` currently
saves a pointer (`Value *`, the anchor value) and an `int`. For most users of LLVM, a pointer will take 8 bytes. Because of alignment,
we will use another 8 bytes anyway for `KindOrArgNo`, not 4. And I think it will be so much simpler to use the wasted 4 bytes
to decouple the values. That is, one `int` for the kind and one `int`for the argument number.

If we all agree in that, of course I'm willing to do the related patches.


I'm actually not really sure for the `value >= 0 ...` part.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76175

Files:
  llvm/include/llvm/Transforms/IPO/Attributor.h
  llvm/lib/Transforms/IPO/Attributor.cpp


Index: llvm/lib/Transforms/IPO/Attributor.cpp
===================================================================
--- llvm/lib/Transforms/IPO/Attributor.cpp
+++ llvm/lib/Transforms/IPO/Attributor.cpp
@@ -8318,7 +8318,7 @@
     switch (I.getOpcode()) {
     default:
       assert((!ImmutableCallSite(&I)) && (!isa<CallBase>(&I)) &&
-             "New call site/base instruction type needs to be known int the "
+             "New call site/base instruction type needs to be known in the "
              "Attributor.");
       break;
     case Instruction::Load:
Index: llvm/include/llvm/Transforms/IPO/Attributor.h
===================================================================
--- llvm/include/llvm/Transforms/IPO/Attributor.h
+++ llvm/include/llvm/Transforms/IPO/Attributor.h
@@ -29,14 +29,14 @@
 // automatically capture a potential dependence from Q to P. This dependence
 // will cause P to be reevaluated whenever Q changes in the future.
 //
-// The Attributor will only reevaluated abstract attributes that might have
+// The Attributor will only reevaluate abstract attributes that might have
 // changed since the last iteration. That means that the Attribute will not
 // revisit all instructions/blocks/functions in the module but only query
 // an update from a subset of the abstract attributes.
 //
 // The update method `AbstractAttribute::updateImpl` is implemented by the
-// specific "abstract attribute" subclasses. The method is invoked whenever the
-// currently assumed state (see the AbstractState class) might not be valid
+// specific "abstract attribute"(AA*) subclasses. The method is invoked whenever
+// the currently assumed state (see the AbstractState class) might not be valid
 // anymore. This can, for example, happen if the state was dependent on another
 // abstract attribute that changed. In every invocation, the update method has
 // to adjust the internal state of an abstract attribute to a point that is
@@ -152,8 +152,8 @@
 
   /// The positions we distinguish in the IR.
   ///
-  /// The values are chosen such that the KindOrArgNo member has a value >= 1
-  /// if it is an argument or call site argument while a value < 1 indicates the
+  /// The values are chosen such that the KindOrArgNo member has a value >= 0
+  /// if it is an argument or call site argument while a value < 0 indicates the
   /// respective kind of that value.
   enum Kind : int {
     IRP_INVALID = -6, ///< An invalid position.
@@ -277,14 +277,7 @@
       return CB->getCalledFunction();
     assert(KindOrArgNo != IRP_INVALID &&
            "Invalid position does not have an anchor scope!");
-    Value &V = getAnchorValue();
-    if (isa<Function>(V))
-      return &cast<Function>(V);
-    if (isa<Argument>(V))
-      return cast<Argument>(V).getParent();
-    if (isa<Instruction>(V))
-      return cast<Instruction>(V).getFunction();
-    return nullptr;
+    return getAnchorScope();
   }
 
   /// Return the associated argument, if any.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D76175.250357.patch
Type: text/x-patch
Size: 2978 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200314/a4753ec6/attachment.bin>