[PATCH] D62555: [TailDuplicator] prevent tail duplication for INLINEASM_BR

Nick Desaulniers via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 29 10:30:01 PDT 2019


nickdesaulniers added a comment.

> Could this be fixed in the if conversion pass IfConverter::ScanInstructions() by having TargetInstrInfo::isPredicable(MachineInstr&) reject TargetOpcode::INLINEASM_BR?

Excellent point.  Thinking more about this last night, and your comment; I think we may have 2 bugs here, and the current patch works but for the wrong reasons.  I should have read through IfConverter instead of just focusing on TailDuplicator.

Towards your point quoted above (and without having looked at IfConverter), I'd naively say that:

- INLINEASM_BR is a pseudo-op at the MIR level; basically a list of assembly instructions specified by the user that should not be peephole optimized.
- Theoretically, we might be able to predicate the block, but we'd basically have to disassemble the user specified inline assembly, and check that all of the instructions were predicable.
- If IfConverter has the machinery and finds it safe to do so for INLINEASM, then we can and should reuse that machinery.
- Otherwise, conservatively do not allow INLINEASM_BR to be predicable. (I'm leaning more towards this, since such machinery would be rewriting the inline asm the user specified).

Either way, the test case can be reused for both issues (with different and improved `CHECK`s.

That's the first issue, observed w/ Debug builds.  The second issue which was first observed with Release builds is that TailDuplicator was leaving behind references to removed branches for INLINEASM_BR.  We can either skip this transformation for MachineBasicBlocks when encountering an INLINEASM_BR (as the patch currently does) with the backwards iteration improvement (you suggested), or I can see if we can update the reference correctly when we duplicate an INLINEASM_BR.  I think what might make the latter difficult is that INLINEASM_BR's operands are of type BlockAddress, not MachineBlockAddress (IIRC, might be wrong).

Thanks for the code review; it definitely provided food for thought and will result in improved fixes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62555





More information about the llvm-commits mailing list