[PATCH] D64258: [InferFuncAttributes] extend 'dereferenceable' attribute based on loads

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Mostly comments to improve this. Two required changes.

Maybe we could mention that this logic should, or better will, move into the Attributor framework somewhere?



================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:44
+    if (!match(&I, m_Load(m_Value(PtrOp))))
+      continue;
+
----------------
"Style": Is this the only use of the "match" function? If so, why not do the (in the middle-end) more familiar pattern of `dyn_cast` and `getPointerOperand`?


================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:51
+    Value *Base = GetPointerBaseWithConstantOffset(PtrOp, ByteOffset, DL);
+    auto *Arg = dyn_cast_or_null<Argument>(Base);
+    if (!Arg || ByteOffset < 0)
----------------
I don't think you can get a `nullptr` back.


================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:61
+    if (!ArgSizeInBits || ArgSizeInBits % 8 != 0)
+      continue;
+
----------------
Maybe a bit more general, something like:
```
bits = DL.getTypeSizeInBits(ArgTy->getType()->getPointerElementType()`
// Round down to the nearest multiple of 8, dereferenceable attributes uses bytes.
bits = bits - bits % 8;
if (!bits)
  continue;
```

(`GEPOperator::accumulateConstantOffset` uses `DL.getTypeAllocSize` which we could probably use as well.)


================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:74
+    SetOfOffsets &OffsetsForArg = ArgOffsetMap[Arg];
+    OffsetsForArg.insert(ByteOffset / (AccessSizeInBits / 8));
+    if (!isGuaranteedToTransferExecutionToSuccessor(&I))
----------------
Two ideas:
1) We could only track minimum + maximum `ByteOffset` values, **iff** we know you cannot "jump" between allocations.
2) Regardless of 1), we could use the maximum `ByteOffset` value we found for a `inbounds` GEP as a lower bound for the `dereferenceable` bytes. `inbounds` GEPs should not allow to do any "jumping" or starting outside the object.


================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:109
+    unsigned EltSize = PtrTy->getElementType()->getPrimitiveSizeInBits();
+    uint64_t DerefBytes = MaxOffset * (EltSize / 8);
+
----------------
Wrt. the above changes it would probably be: `MaxOffset * EltSize / 8`.


================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:115
+    if (F.getParamDereferenceableBytes(ArgNumber) < DerefBytes) {
+      F.removeParamAttr(ArgNumber, Attribute::Dereferenceable);
+      F.addDereferenceableParamAttr(ArgNumber, DerefBytes);
----------------
You need to remove `deref_or_null` as well.


================
Comment at: llvm/test/Transforms/InferFunctionAttrs/dereferenceable.ll:114
 define void @volatile_can_trap(i16* %ptr) {
-; CHECK-LABEL: @volatile_can_trap(i16* %ptr)
+; CHECK-LABEL: @volatile_can_trap(i16* dereferenceable(2) %ptr)
   %arrayidx0 = getelementptr i16, i16* %ptr, i64 0
----------------
volatile should not cause `deref`, I think this was said:

>  This means the compiler may not use a volatile operation to prove a non-volatile access to that address has defined behavior.



================
Comment at: llvm/test/Transforms/InferFunctionAttrs/dereferenceable.ll:143
 
-; Could round weird bitwidths down?
+; TODO: Could round weird bitwidths down?
 
----------------
Yes ;)


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

https://reviews.llvm.org/D64258