[PATCH] D47441: SafepointIRVerifier should ignore dead blocks and dead edges

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/IR/CFGDeadness.cpp:127
+      const Use &U = PU.getUser()->getOperandUse(PU.getOperandNo());
+      assert(!result &&
+             "Phi is not expected to refer to one block several times");
----------------
yrouban wrote:
> anna wrote:
> > I think the assert and the corresponding ifdef compilation with break isn't very useful. It's just testing a property of the IRVerifier for phi nodes. 
> > Also, conditional compilation with behaviour change is not very preferable. Could we get rid of this?
> In debug mode these two assertions (1) and (2) check that for this phi node and the specified basic block there is at leats one edge (2) and no more than one edge (1). To check (1) we need to iterate through the whole set of predecessors, that is why we should not break at the first found edge. Any other way to express this more elegantly?
> This assertion seems obvious, but I did not find a place in LLVM (other than the LL parser) that prevents phis with duplicate pairs like phi [%value1, %block1], [%value1, %block1]. In this case, how many incoming edges do we have, 1 or 2?
Yes, I got what you were doing :)
I incorrectly thought this exact case is handled in the IR verifier (i.e. it fails with dup blocks), so we don't need to do this here. But this is infact valid IR (having duplicate pairs will be 2 edges, i.e. 2 preds for that basic block). 
So, now that I think of it since the verifier allows it, the IR is valid - so why should we have this assert in the first place? 




https://reviews.llvm.org/D47441