[PATCH] D81929: [WIP][Attributor] Introduce CallBaseContext to the IRPosition

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D81929#2129807 <https://reviews.llvm.org/D81929#2129807>, @kuter wrote:

> Btw this patch introduces 8 bytes per attribute because of the `CBContext`.
>
> [...]
>
> Any other ideas ?


Run heapcheck and if the cgscc memory usage is not impacted much keep it this way ;)

We need a test for this. We can use unit tests or check the printed output (lit test + `requires: asserts` IIRC). Either works but we should add one.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:457
+    result.Enc = Enc;
+    return result;
+  }
----------------
I'd prefer to copy the entire thing and then `null` the context. Will be optimized to the same code but is future proof :)


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:462
+
+  bool hasCallBaseContext() const { return CBContext != nullptr; }
+
----------------
Documentation for all of them please.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:588
+  /// CallBase context. Used for callsite specific analysis.
+  const CallBaseContext *CBContext;
+
----------------
Nit: I would prefer to move/add the initialization here. Assuming it is all the same to you.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:936
+    }
+#endif
+    const IRPosition RealIRP =
----------------
Move this into a helper function so we don't have to "make LLVM_DEBUG" work here.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:938
+    const IRPosition RealIRP =
+        shouldPropagateCallBaseContext(IRP) ? IRP : IRP.stripCallBaseContext();
+
----------------
`& RealIRP` or maybe:
```
if (!should...)
  IRP = IRP.strip...
```


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:90
+    cl::desc("Allow the Attributor to do call site specific analysis"),
+    cl::init(false));
+
----------------
Nit: `attributor-enable-call-site-specific` is missing a final word, maybe `specialization` or `deduction` or something. 


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:465
+    assert((CBContext == nullptr) &&
+           "'call site argument' position must not have CallBaseContext!");
     Use *U = getAsUsePtr();
----------------
Good thinking!



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:803
+  // a
+  // Instruction that would effect the liveness/return state etc.
+  return EnableCallSiteSpecific;
----------------
Nit, formatting.


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

https://reviews.llvm.org/D81929