[PATCH] D100094: [ARM] Simplification to ARMBlockPlacement Pass.

[Malhar via Phabricator via llvm-commits]

malharJ added inline comments.


================
Comment at: llvm/test/CodeGen/Thumb2/block-placement.mir:17-20
-  define void @backwards_branch_target_already_backwards(i32 %N, i32* nocapture %a, i32* nocapture readonly %b) local_unnamed_addr #0 {
-  entry:
-    unreachable
-  }
----------------
malharJ wrote:
> This test checked that the loopExit/target block is unaffected when moved forwards,
> if it already contains a backwards WLS.
> 
> However, it has been removed since the updates in this patch imply that
> the loopExit/target is no longer moved around.
Instead of removing this, I've now replaced this with an equivalent test:  **backwards_branch_forwards_wls**.

(The naming is as such because there is already another test called **backwards_branch_backwards_wls**, which 
tests the opposite scenario)


================
Comment at: llvm/test/CodeGen/Thumb2/block-placement.mir:22-25
-  define void @backwards_branch_sibling(i32 %N, i32 %M, i32* nocapture %a, i32* nocapture %b, i32* nocapture %c) local_unnamed_addr #0 {
-  entry:
-    unreachable
-  }
----------------
samtebbs wrote:
> samtebbs wrote:
> > malharJ wrote:
> > > This test was removed because it didn't seem correct/intent was not clear.
> > > The two blocks containing the t2WhileLoopStartLR have exit branches to each other.
> > > 
> > > Just thought it might be better to remove it as the intent is unclear.
> > > Otherwise, happy to leave it in if required.
> > Sorry for not looking at this sooner. The `backwards_branch_sibling` test made sure that if a backwards WLS (bb3 -> bb1) in this case was introduced by moving the loop exit (bb1) then no transformation would occur. It would still be useful to include this but adapt it to make sure that no moving happens if it would create a forwards LE.
> I meant to write "backwards WLS (bb3 -> bb1 in this case) was introduced..."!
Thanks for explaining this, but there are a few things I'd like to mention:

1) I've added a test (for checking that backwards WLS is not introduced): **backwards_branch_target_already_backwards**.
(It was essentially a replacement for the older test named **backwards_branch_forwards_le**)

So would it be ok to not include **backwards_branch_sibling** test since it's already being tested ?

2)  

> It would still be useful to include this but adapt it to make sure that no moving happens if it would create a forwards LE.

With the new logic, a forward LE cannot be introduced since only the PreHeader is being moved backwards.
Any LE to it will remain backwards, and there is no chance of it containing a LE (since it already contains WLS which is a terminator) unless Im mistaken there.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100094