[PATCH] D117241: [Attributor] Share code for abstract interpretation of allocation sizes with getObjectSize [NFC-ish]

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:364
+  const ConstantInt *Arg =
+    dyn_cast<ConstantInt>(Mapper(CB->getArgOperand(FnData->FstParam)));
+  if (!Arg)
----------------
jdoerfert wrote:
> nit: dyn_cast_or_null or document the mapper should not return a nullptr. I prefer the former.
I strongly prefer not allowing the mapper to return null for no reason.

The wording in the comment is "replace one Value* (operand to the allocation) with another"  This already implies the result can't be null to me.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:6022
+      bool Dead = false;
+      if (Optional<Constant *> SimpleV = A.getAssumedConstant(*V, AA, Dead))
+        if (*SimpleV)
----------------
jdoerfert wrote:
> Nit: Not "Dead" but "UsedAssumedInformation". That will need to be exposed once we want to "fix" AllocationInfos in H2S early to avoid looking at them again in future updates. For now, I'd just use the canonical name here.
Bryce already got this.

Though I'll note that this appears to be a dead outparam, and the more general mechanism a bunch of code complexity which is unused.  (I haven't looked at the general case closely, I might be missing something.)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117241