[PATCH] D64258: [InferFuncAttributes] extend 'dereferenceable' attribute based on loads
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 9 21:56:49 PDT 2019
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
More information about the llvm-commits
mailing list