[PATCH] D33533: [IR] Add an iterator and range accessor for the PHI nodes of a basic block.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: include/llvm/IR/BasicBlock.h:286
+
+    bool operator==(const phi_iterator_impl &Arg) const { return PN == Arg.PN; }
+
----------------
chandlerc wrote:
> dblaikie wrote:
> > Generally prefer to use non-member op overloads where possible (but maybe the iterator_facade isn't compatible with this? Not sure). That way LHS and RHS conversions happen equally - relevant for, say, const+non-const_iterator comparisons.
> I have no idea, but all the uses of iterator_facade_base follow this pattern. I'd somewhat rather not try to fix this in this patch....
Rightio


================
Comment at: include/llvm/IR/BasicBlock.h:293
+      assert(PN && "Cannot increment the end iterator!");
+      PN = dyn_cast<PHINodeT>(std::next(BBIteratorT(PN)));
+      return *this;
----------------
chandlerc wrote:
> dblaikie wrote:
> > Seems weird to build an iterator in every increment, though I imagine it's cheap - any reason to do this rather than to keep the iterator as a member (rather than the pointer as a member)?
> It avoids the dyn_cast on every dereference (or having more state).
Presumably only a cast, not a dyn_cast, right? (isa on increment, cast on deref?)

but sure


================
Comment at: include/llvm/IR/BasicBlock.h:304-306
+  iterator_range<const_phi_iterator> phis() const {
+    return const_cast<BasicBlock *>(this)->phis();
+  }
----------------
chandlerc wrote:
> dblaikie wrote:
> > Usually I expect to see this done the other way around - having the non-const version delegate to the const version. Otherwise the const version is technically UB when invoked on a const object (since it casts away constness, calls a function, etc). Though I realize that means needing an explicit conversion from const iterator to non-const iterator - maybe easier to have two implementations then, given it's short/simple.
> Why is it UB to cast away const even on a const object? I don't think anything makes that UB.
> 
> The UB thing is to *modify* a const object. But the non-const function here doesn't do that, so it seems fine?
> 
> I can have two implementations, but then I have to put them both out-of-line because the implementation of this method cannot be written in this header (requires the definition of the PHINode type which requires the definition of the BasicBlock type...)
Fair - can't find anything that says it's UB to call a non-const function on a const object, but weirds me out a bit because it'd make it pretty easy to leak a non-const pointer/mutate state by accident, so I'd tend to prefer the inverse phrasing, but I realize it's a bit awkward. Up to you.


================
Comment at: unittests/IR/BasicBlockTest.cpp:67
+  // And iterate a const range.
+  for (auto &PN : const_cast<const BasicBlock *>(BB.get())->phis()) {
+    EXPECT_EQ(BB.get(), PN.getIncomingBlock(0));
----------------
chandlerc wrote:
> dblaikie wrote:
> > const_casts /to/ const kind of weird me out (because they're rare & so I'm only really used to seeing const_casts removing const - takes a bit to read when they're adding it). Should we have an llvm::implicit_cast?
> > 
> > Should it be "const auto &" on the LHS, if this is a const iterator?
> To the first point: what would you suggest I write? I don't really care, this seemed to work.
> 
> To the second point: why? auto will deduce const just fine... adding const myself won't really surface any bugs. I guess I always prefer to write less when there is no reason to write more...
I'd love an implicit_cast - if you want to add one while you're at it, but I understand that's a whole nother thing with test cases, etc.

I figure const, &, and * are short enough that the relative cost/benefit is worthwhile to include them. The LLVM style guide does at least call for '*' when auto is deducing to a pointer (/when that matters - if it's some opaque typedef that the code doesn't care about pointerness, I suppose '*' wouldn't be used) & I've always assumed (incorrectly, now that I look) that the style guide said the same about const. At least that's my habit.

Up to you.



https://reviews.llvm.org/D33533