[PATCH] D157068: [Attributor] New attribute to identify what byte ranges are alive for an allocation

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12476
+    const AAPointerInfo *PI =
+        A.getOrCreateAAFor<AAPointerInfo>(IRP, *this, DepClassTy::OPTIONAL);
+
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12484
+
+    const auto &OtherAAImpl = static_cast<const AAPointerInfoImpl &>(*PI);
+    const auto &State = OtherAAImpl.getState();
----------------
Don't cast it to an "Impl". There is no need anymore anyway.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12485
+    const auto &OtherAAImpl = static_cast<const AAPointerInfoImpl &>(*PI);
+    const auto &State = OtherAAImpl.getState();
+
----------------
State could be anything. At least call it PIState.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12489-12495
+    if (BinSize > 1)
+      return indicatePessimisticFixpoint();
+
+    const auto &It = State.begin();
+
+    if (It == State.end())
+      return indicatePessimisticFixpoint();
----------------
This is basically BinSize != 1, return, no?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12508
+    if (OffsetEnd == AllocationSize)
+      return indicatePessimisticFixpoint();
+
----------------
Why does this not trigger for the ptr/i64 case? Is one of them in bytes the other in bits?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12511
+    Type *IntegerSizeOfOffsetEnd =
+        Type::getIntNTy(AI->getContext(), OffsetEnd * 8);
+
----------------
This is fine for OffsetEnd = 2,4,8, but then it becomes a little iffy. Check for non default sizes and go with a char array instead.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12529
+    Type *IntegerSizeOfAlloca =
+        Type::getIntNTy(AI.getContext(), getAllocatedSize()->getFixedValue());
+
----------------
Same as above.


================
Comment at: llvm/test/Transforms/Attributor/value-simplify-local-remote.ll:82
 ; CGSCC-NEXT:  entry:
-; CGSCC-NEXT:    [[QQFIRST_ADDR:%.*]] = alloca ptr, i32 0, align 8
 ; CGSCC-NEXT:    store ptr [[QQFIRST]], ptr [[QQFIRST_ADDR]], align 8
----------------
This doesn't actually make it smaller, we should avoid such changes.


================
Comment at: llvm/test/Transforms/Attributor/value-simplify-local-remote.ll:135
 ; TUNIT-NEXT:  entry:
-; TUNIT-NEXT:    [[RETVAL:%.*]] = alloca [[S:%.*]], i32 0, align 8
 ; TUNIT-NEXT:    store ptr [[FOO_THIS]], ptr [[FOO_THIS]], align 8
----------------
Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157068