[PATCH] D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11064
+
+  while (true) {
+    for (SDNode::use_iterator UI = Addr.getNode()->use_begin(),
----------------
efriedma wrote:
> rengolin wrote:
> > evgeny777 wrote:
> > > rengolin wrote:
> > > > I'm worried about this... there should be a guaranteed exit condition here.
> > > What about reducing number of iterations to, let's say, 16? This should be more than enough for most of cases. Also the effect of compilation time increase should become limited.
> > I was hoping for an approach that didn't need O(n^2) even if lower bound. If your increment is 16x, then the loop will be 16x less efficient than previously. Granted, it's doing more, but that's a big impact that I'd rather avoid.
> > 
> > But I have been away from isel for quite a while now, so I'll wait for some current isel developers (@qcolombet @rovka) to shed some light.
> I'll have to think about this a bit more... but I'll just briefly note that the existing CombineBaseUpdate has shown up in the past as a compile-time hog for large basic blocks.  (I think it's worst-case O(N^3) or something like that.)
Thanks Eli. I just want to make sure that we only do that if we really need it, ie. the benefit outweigh the costs/risks and there is no other way.


Repository:
  rL LLVM

https://reviews.llvm.org/D39415