[PATCH] D116971: [AttributorAttributes] Remove hardcoded parameters

[Bryce Wilson via Phabricator via llvm-commits]

Bryce-MW added inline comments.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:314
+llvm::getAllocSizeArgs(const CallBase *V, const TargetLibraryInfo *TLI) {
+  assert((isMallocOrCallocLikeFn(V, TLI) || isReallocLikeFn(V, TLI)) &&
+         "getAllocSizeArgs can only be called on an allocation function that "
----------------
reames wrote:
> Please replace this with assert(isAllocationFn()).  There's no reason we can't return a result for e.g. op new.  The current client won't use it, but we should be generic.  
I believe MallocOrCallocLike already includes op new.
```
 MallocLike         = 1<<1 | OpNewLike
```
My intention was to include everything but StrDupLike since it has size parameters that work differently.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:5840
+
+      auto *AllocValue = getInitialValueOfAllocation(CB, TLI, CB->getType());
+      AllocationInfo::AllocationKind Kind;
----------------
reames wrote:
> Can be null in general, might be prevented by the kind precondition, might not.  Haven't looked.  
I had meant for null to be a possibility. That would be the only case where the else would happen.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:5843
+      if (AllocValue->isNullValue()) {
+        Kind = AllocationInfo::AllocationKind::AllocZeros;
+      } else if (isa<UndefValue>(AllocValue)) {
----------------
reames wrote:
> I think all you need in client code is a boolean IsZeroInit.  
Good point


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:5989
       // Zero out the allocated memory if it was a calloc.
-      if (AI.Kind == AllocationInfo::AllocationKind::CALLOC) {
+      if (AI.Kind == AllocationInfo::AllocationKind::AllocZeros) {
         auto *BI = new BitCastInst(Alloca, AI.CB->getType(), "calloc_bc",
----------------
reames wrote:
> I might be missing something, but I think you can simplify/generalize this code by querying the init value as an i8 before deleting the call using AI.CB, use that value in the memset, and then just check that the value being memset is not undef/poison.  (The last check is in fact optional - it's a minor compile time win.)
> 
> I think this removes the need for the whole alloc state enum, and upfront detection.
The main reason for the early detection is if getInitialValueOfAllocation returned null, then we wouldn't be able to remove the call. But maybe it makes sense to do it twice. Once to see if we know the initial value, and once to see what to memset. That's probably a better design (in case we ever have an allocation function that allocates all FF for some reason or something like that). I'll change that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116971