[PATCH] D144445: [ConstantFold][InstSimplify] folding load for constant global patterened arrays and structs

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

leave some comments to consider.



================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:773
+// constantness and struct type.
+uint64_t GetMinimumGEPStride(Value *PtrOp, const DataLayout DL) {
+
----------------
Is it better to implement on `GEPOperator`?


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:798
+  unsigned LoadSize = LoadTy->getScalarSizeInBits() / 8;
+  if (LoadAlign <= GVSize)
+    for (unsigned ByteOffset = LoadAlign, E = GVSize - LoadSize;
----------------
khei4 wrote:
> nikic wrote:
> > If the load alignment is used, you also need to make sure that the global alignment is `>=` the load alignment. Otherwise it doesn't really tell you anything.
> > 
> > Though I'm not sure we really want to use alignment to determine access stride -- this is pretty unusual. Based this on the getelementptr stride would be more typical and handle cases that alignment can't (e.g. non-power-of-two stride).
> > If the load alignment is used, you also need to make sure that the global alignment is >= the load alignment. Otherwise it doesn't really tell you anything.
> 
> Right. I'll handle that case. 
> 
> > Based this on the getelementptr stride would be more typical and handle cases that alignment can't (e.g. non-power-of-two stride)
> 
> Ok, I am a little confused. Overall I thought I don't need to care about the case that alive decides any value could be returned. Although I didn't,  `undef` value could be returned?
> 
> - if `Global Variable's alignment` > `load's alignment`, then whatever offsetted pointer is given to load, then any value could be returned.
> 
> https://alive2.llvm.org/ce/z/5_EDLB
> 
> - even if `Global Variable's alignment` <= `load's alignment`, the offset is not the multiple of load's alignment, then any value could be returned.
> 
> https://alive2.llvm.org/ce/z/Qnon4K
> 
> based on these assumptions, I used alignment to calculate possible valid accessible offsets. Are these assumptions and alive results correct?
> I might have misunderstood or broadly interpret alive results.
> 
> But I'm feeling it's also good to compute stride from GEP and use a bigger one. A possible minimum stride can be gained by calculating the greatest common divisor of GEP-type sizes, so it's simpler than I expected.
> (This might be out of scope, it might be better not to use GEP idx, I saw [[ https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699 | your proposal ]]  )
Is the following case the example that only the seeing only GEP stride can't fold possible case?
https://alive2.llvm.org/ce/z/9cVcee


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

https://reviews.llvm.org/D144445