[PATCH] D88823: [CodeGen][TailDuplicator] Don't duplicate blocks with INLINEASM_BR

Tue Oct 6 00:11:47 PDT 2020

nickdesaulniers added a comment.

Thanks for the additional context.  I //assert// that that fuller demonstration does indeed have a bug in tail duplication, one that is //not// exposed by the current test case, so @jyknight may be correct that the current test case was over reduced.  From the `After tail duplication` linked above <https://reviews.llvm.org/D88823#2313373>:

Take a look at the `PHI` in `bb.19.bb17.i.i.i`.  The values it gets (via the indirect branch of the `INLINEASM_BR`) are garbage.  Pared down to highlight what I'm referring to:

  bb.16:
    ...
    INLINEASM_BR ; conditional jump to %bb.19
    %60:gr64 = COPY %13:gr64
    JMP_1 %bb.20

  bb.17:
    ...
    INLINEASM_BR ; conditional jump to %bb.19
    %61:gr64 = COPY %4:gr64
    JMP_1 %bb.20

  bb.19:
    %62:gr64 = PHI %60:gr64, %bb.16, %61:gr64, %bb.17
    ...

Either way, if we take the indirect branch (via the assembly within the `INLINEASM_BR`), whatever the results of the `PHI` of `%62` resolve to are going to be hella wrong, since the `COPY`s occurred //after// the `INLINEASM_BR`s.  Instead, the `COPY`s should have occurred //before// the `INLINEASM_BR`.

My dump of the current test case post tail duplication above <https://reviews.llvm.org/D88823#2312933> contains no such `COPY` related bug.  I think if the test case was updated to demonstrate that, then we'd have a starting point to discuss whether we can properly resolve `PHI`s or not.

---

That we don't have a `-verify-machineinstrs` pass that detects that case is also 💩 💩 💩 .  We should be able to walk backwards up the use-def chain of any `PHI` and find a `COPY` or `def`, I suppose.

---

> we shouldn't be trying to shove those copy instructions before the INLINEASM_BR, ...

Disagree.  Without doing so we've generated garbage.

> ... because that's contrary to how we normally resolve PHI nodes.  The back-end relies upon this.

I'll take your word on that.  TBH, I don't understand why the `COPY`'s are necessary...why don't we just transform `%62` to `%62 = PHI %13, %bb.16, %4, %bb.17`?  Trying to minimize live ranges, perhaps?

> So if we allow for PHI node resolution from the indirect branch, and the subsequent instructions are placed before the asm goto block, then it's violating that assumption.

Not "[all of] the subsequent instructions," can we //just// move the newly created `COPY` to be before the `INLINEASM_BR`?

> Not only is the code slower, but it may result in incorrect execution.

How can hoisting `COPY`s to be above the `INLINEASM_BR` that might branch to a block whose `PHI` will use the result lead to incorrect execution?  Slower, //maybe//, but then I think my question about `%62` would help there.

---

Wait, @void, was your above MIR <https://reviews.llvm.org/D88823#2313373> dumped with f7a53d82c0902147909f28a9295a9d00b4b27d38 <https://reviews.llvm.org/rGf7a53d82c0902147909f28a9295a9d00b4b27d38> not applied, perhaps?  Say if you had `git checkout 4b0aa5724fea` for instance to test the results of the initial "`INLINEASM_BR` is no longer a terminator" change?  Oh, `TailDuplicator` does not use `findPHICopyInsertPoint`, so it may have exactly the same bug!

Yeah! I was looking at `TailDuplicator::appendCopies` before thinking "I bet `MachineBasicBlock::iterator Loc = MBB->getFirstTerminator();` behaves differently after @jyknight 's 4b0aa5724fea <https://reviews.llvm.org/rG4b0aa5724feaa89a9538dcab97e018110b0e4bc3>."  What if that was updated with sauce from `findPHICopyInsertPoint`?  (It would need to know the successor BB as it's only given the predecessor BB currently).  I bet that's it.  It would be nice to reuse `findPHICopyInsertPoint` if possible (we may not want to hoist //all// `COPY`s in `CopyInfos`, but I'm not sure).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88823