[PATCH] D147982: Account for PATCHABLE instrs in Branch Relaxation

[Daniel Hoekwater via Phabricator via llvm-commits]

dhoekwater added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:1653
   // expands to special code sequences which must be present.
   auto First = MBB.getFirstNonDebugInstr();
+  if (First == MBB.end())
----------------
paquette wrote:
> I think this could be a `llvm::find_if`?
> 
> ```
> return find_if(MBB.getFirstNonDebugInstr(), MBB.getLastNonDebugInstr(), [](const MachineInstr &MI) {
>    unsigned Opc = MI.getOpcode();
>    switch(Opc) {
>       default:
>           return false;
>       case TargetOpcode::FENTRY_CALL:
>       ...
>          return true;
>    }
> }) != MBB.end();
> ```
> 
> That'd make it easy to add new opcodes/remove opcodes.
It definitely could be, and it makes the function visually a lot cleaner. Because PATCHABLE instructions are always inserted at the start or at/just before the end, the previous and current implementations are O(1) in the number of instructions.

Being fairly unfamiliar with the performance characteristics of the codebase, I was hesitant to scan through the whole basic block. If the small performance difference isn't a concern, I'd be happy to switch to `llvm::find_if`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147982