[PATCH] D104432: [Attributor] Introduce AAPointerInfo

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:835-838
+    return (OAS.getOffset() >= getOffset() &&
+            OAS.getOffset() < getOffset() + getSize()) ||
+           (getOffset() >= OAS.getOffset() &&
+            getOffset() < OAS.getOffset() + OAS.getSize());
----------------
kuter wrote:
> I think range intersection can be simpler here 
> 
> https://stackoverflow.com/questions/3269434/whats-the-most-efficient-way-to-test-two-integer-ranges-for-overlap
simplified :)


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1012-1013
+
+  /// See AAPointerInfo::forallInterfearingAccesses.
+  bool forallInterfearingAccesses(
+      Instruction &I,
----------------
kuter wrote:
> This function is going to be called from the `updateImpl` method of a attribute.
> Maybe it would make sense to make it a little faster at least in some common cases or maybe some caching ?
> 
Can we postpone this as it will be more meaningful as we start using AAPointerInfo. Then we can actually measure any "optimization" here properly.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1034
+    // Now that we have an offset and size, find all overlapping ones and use
+    // the callback on the accesses.
+    for (auto &It : AccessBins) {
----------------
kuter wrote:
> Maybe it would be worth handling the case where the access is always aligned ?
> like `int array[100];`
> 
> Maybe we can have a flag inside the state that tells us that all the load/store instructions that we looked at have aligned access.  
> Then we can just use the offset (maybe through a hashmap lookup) to find any interfering access.
> 
> Or maybe  if we have  sorted set like std::set (sorted by offset). We can limit where we end our scan.
> We would only scan [0, OAS.getOffset() + OAS.getSize()].
> 
> Or even better if we have  maximum access size for all access
> We can just scan just scan the range [OAS.getOffset() - MaxSize, OAS.getOffset() + OAS.getSize()].
> 
> 
The bins need a lot more logic for this but I can see that it can help. We basically would want to store what the modulo is between offset and base. That would even allow us to handle non-static offsets much better if we know they come from a specific multiple, that is `gep .. %i`. That said, I'd prefer to postpone such optimizations for later.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1102
+            RAcc.getContent(), CB, *this, UsedAssumedInformation);
+        AAPointerInfo::Access TranslatedAcc(&CB, Content, RAcc.getKind(),
+                                            RAcc.getType());
----------------
kuter wrote:
> Maybe we can use  `addAccess` here  ? 
Good catch. Did that :)


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1203
+            !GEP->hasAllConstantIndices()) {
+          UsrOI.Offset = OffsetAndSize::Unknown;
+        } else {
----------------
kuter wrote:
> Maybe an early return here would be easier to read ?
> 
> 
will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104432