[PATCH] D131316: [LoongArch] Implement branch analysis

[Ray Wang via Phabricator via llvm-commits]

wangleiat added a comment.

In D131316#3704488 <https://reviews.llvm.org/D131316#3704488>, @barannikov88 wrote:

> Just passed by, you can ignore my comments.

Thanks for your comments.



================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp:168
+  // If AllowModify is true, we can erase any terminators after
+  // FirstUncondOrIndirectBR.
+  if (AllowModify && FirstUncondOrIndirectBr != MBB.end()) {
----------------
barannikov88 wrote:
> How is this possible to have instructions after an unconditional terminator? I think it should be asserted that there are none.
Agree with you, but this situation really needs to be handled here. Because`insertBranch`  will add an unconditional branch with `BranchFolder` [[ https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/BranchFolding.cpp#L1692 | pass ]].



================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp:179
+  // in an indirect branch.
+  if (NumTerminators > 2 || I->getDesc().isIndirectBranch())
+    return true;
----------------
barannikov88 wrote:
> This check seems superfluous, the code below will filter out this case.
Yes, I will remove it.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp:216
+      !I->getDesc().isConditionalBranch())
+    return 0;
+
----------------
barannikov88 wrote:
> This is impossible condition, because this function is only called if analyzeBranch returned success, and that returns success only if the last instruction was an (un-)conditional branch.
> 
`analyzeBranch` will return success when the block has no branches (nothing to be removed).

`isBranch()` condition maybe ok?

```
if (!I->getDesc().isBranch())
    return 0;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131316