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

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 3 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:40
+    Value *PtrOp;
+    if (!match(&I, m_Load(m_Value(PtrOp))))
+      continue;
----------------
hfinkel wrote:
> Why not also stores (or AtomicCmpXchg or AtomicRMW)?
Oversight on my part - tunnel vision based on the motivating cases. 

I've never done anything with the atomic opcodes, so I didn't remember them. For stores, how would dereferenceable aid optimization?
Ok if we make this a TODO enhancement/follow-up?



================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:44
+    // The implicit GEP index for that pattern is zero.
+    auto *Arg = dyn_cast<Argument>(PtrOp);
+    uint64_t GepIndex = 0;
----------------
hfinkel wrote:
> This logic seems unnecessarily limited. Why not use GetPointerBaseWithConstantOffset?
Another oversight on my part. I wasn't thinking about cases with pointer casts, so that made the logic simpler. Will change, but hoping a partial implementation is good enough for an initial patch (see next comment).


================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:87
+    for (uint64_t Index : GepIndexes) {
+      if (Index != MaxGepIndex)
+        break;
----------------
hfinkel wrote:
> Does this do the right thing if the lowest GEP index is negative? You could skip the negative ones first?
> 
> As a general point, I think that you want logic here mirroring (a subset of) what's in isOverwrite in DeadStoreElimination.cpp
I don't think there was miscompile potential there, but that was accidental. I didn't think about negative offsets. Will fix.
I'd like to make the DSE-like enhancement to support arbitrary-sized sub-ranges (via pointer casts) a follow-up, so this patch doesn't get too complicated.


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

https://reviews.llvm.org/D64258