[PATCH] D41006: [SafepointIRVerifier] Allow non-dereferencing uses of unrelocated or poisoned PHI nodes
Anna Thomas via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 21 09:53:33 PST 2017
anna added inline comments.
================
Comment at: lib/IR/SafepointIRVerifier.cpp:491
+
+ if (isNotExclusivelyConstantDerived(InValue)) {
+ if (isValuePoisoned(InValue)) {
----------------
anna wrote:
> DaniilSuchkov wrote:
> > anna wrote:
> > > Instead of the below code (logic is right, but too many conditionals), could we do something like this:
> > > ```
> > > if (isNotExclusivelyConstantDerived(InValue)) {
> > > if (isValuePoisoned(InValue) || (HasRelocatedInputs && HasUnrelocatedInputs)) {
> > > PoisonedPointerDef = true;
> > > break;
> > > }
> > > if (BlockMap[InBB]->AvailableOut.count(InValue))
> > > HasRelocatedInputs = true;
> > > else
> > > HasUnrelocatedInputs = true;
> > > }
> > > if (!PoisonedPointerDef && HasUnrelocatedInputs) {
> > > assert(!HasRelocatedInputs && "Should be poisoned!");
> > > ValidUnrelocatedPointerDef = true;
> > > }
> > > ```
> > >
> > But we still have to handle case when some inputs are relocated and some are not: your code won't work if the _last_ input makes this phi poisoned (because on each iteration it checks flags that might have changed on previous one). Thus we cannot remove this part:
> > ```
> > if (HasUnrelocatedInputs) {
> > if (HasRelocatedInputs)
> > PoisonedPointerDef = true;
> > else
> > ValidUnrelocatedPointerDef = true;
> > }
> > ```
> > So the only thing that might be changed is this part:
> > ```
> > if (isValuePoisoned(InValue)) {
> > // If any of inputs is poisoned, output is always poisoned too.
> > HasRelocatedInputs = true;
> > HasUnrelocatedInputs = true;
> > break;
> > }
> > ```
> > But if we'll change it to
> > ```
> > if (isValuePoisoned(InValue)) {
> > // If any of inputs is poisoned, output is always poisoned too.
> > PoisonedPointerDef = true;
> > break;
> > }
> > ```
> > We'll have to somehow tell that `if` (mentioned before) not to touch this flag and it'll be even worse.
> > And that `assert` shouldn't be moved to PHI's branch, it's about all instructions.
> >
> > Currently this part is pretty clear and straightforward: loop over phi's inputs sets two flags (that have clear meaning) and after that loop another two flags are changed accordingly.
> ah yes, it wont work for the last case.
also, pls point out as a comment where these rules come from (i.e. refer to header for reasoning of these rules).
https://reviews.llvm.org/D41006
More information about the llvm-commits
mailing list