[PATCH] D138977: [IR][NFC] Adds BasicBlock::splice().

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:466
+    auto FromItLast = std::next(FromIt);
+    if (ToIt == FromIt || ToIt == FromItLast)
+      return;
----------------
vporpo wrote:
> vporpo wrote:
> > vporpo wrote:
> > > aeubanks wrote:
> > > > vporpo wrote:
> > > > > aeubanks wrote:
> > > > > > vporpo wrote:
> > > > > > > aeubanks wrote:
> > > > > > > > aeubanks wrote:
> > > > > > > > > should add a test for this, e.g. `splice(I1, I2)` where `I1` is right before `I2` in the same block
> > > > > > > > and does the actual implementation not handle this?
> > > > > > > I am not sure. Since I am basically calling splice() internally I was relying on it being well tested. But anyway, since this is part of the BB API now, I agree that it needs a test.
> > > > > > if `getInstList().splice()` already handles this, then I'd say no need for this early return. would be good to double check
> > > > > I just checked and it won't handle this, but I don't think we can handle this either. The current behavior is that it just crashes when the iterator goes out of bounds as it never finds the End iterator. I added a test that confirms the crash.
> > > > > 
> > > > > I don't think we can handle this any better. We could add a linear-time check to test whether BeginIt is before EndIt, but I am not sure it is worth it. What do you think?
> > > > I think adding an assert, even if it's linear time, would be good to help future users. can add it under expensive checks (and gate the death test too) if it's too expensive in assert builds
> > > Yes the ilist splice already early returns if the iterators are equal. I removed the check from BasicBlock::splice(). I will update the patch.
> > Hmm turns out that not all ilist splice functions check this:
> > ```
> >   void splice(iterator where, iplist_impl &L2, iterator first) {
> >     iterator last = first; ++last;
> >     if (where == first || where == last) return; // No change                                                                                                                    
> >     transfer(where, L2, first, last);
> >   }
> >   void splice(iterator where, iplist_impl &L2, iterator first, iterator last) {
> >     if (first != last) transfer(where, L2, first, last);
> >   }
> > ```
> > I am calling the second one which does not check `where == first` or `where == last` and it crashes.
> > I can't think of a reason why the check should be missing. I will create a separate patch for this.
> There seems to be a discrepancy in the way ilist's splice() works. The single-element splice() accepts a destination that is equal to the source. There is even a test on this `TEST(IListTest, SpliceOne)` which checks it.  The multi-element splice() will crash, even if it is transferring only one element. There seems to be a lot of code relying on that, so changing this behavior needs some extended cleanup.
> 
> I think we should try to maintain this functionality for BB splice(), so we should keep the early exit check`if (ToIt == FromIt || ToIt == FromItLast)` in BasicBlock's single-element splice().
in that case can't we just forward every call to `getInstList().splice(..., From->getInstList(), ...)` rather than going through the newly added 4 param `BasicBlock::splice()`? Or do you want to funnel then all through one method so you can keep track of all splice calls more easily?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138977