[ARM][ConstantIslands] Correct MinNoSplitDisp calculation

[pzhengqc]

MinNoSplitDisp was first introduced in D16890 to handle cases where the ConstantIslands pass fails to converge in the presence of big basic blocks. However, the computation of the variable seems to be wrong as it currently computes the offset immediately following UserBB. In other words, it represents the distance from the beginning of the function to the end of UserBB. The distance from the beginning of the function does not seem to be a good indicator of how big the basic block is unless the basic block is close to the beginning of the function. I think MinNoSplitDisp should compute the distance between UserOffset and the end of UserBB instead.

[llvmbot]

@llvm/pr-subscribers-backend-arm

Author: None (pzhengqc)

Changes

MinNoSplitDisp was first introduced in D16890 to handle cases where the ConstantIslands pass fails to converge in the presence of big basic blocks. However, the computation of the variable seems to be wrong as it currently computes the offset immediately following UserBB. In other words, it represents the distance from the beginning of the function to the end of UserBB. The distance from the beginning of the function does not seem to be a good indicator of how big the basic block is unless the basic block is close to the beginning of the function. I think MinNoSplitDisp should compute the distance between UserOffset and the end of UserBB instead.

---

Full diff: https://github.com/llvm/llvm-project/pull/114590.diff

1 Files Affected:

• (modified) llvm/lib/Target/ARM/ARMConstantIslandPass.cpp (+2-1)

diff --git a/llvm/lib/Target/ARM/ARMConstantIslandPass.cpp b/llvm/lib/Target/ARM/ARMConstantIslandPass.cpp
index 1312b44b49bdcc..e0575f7c125f67 100644
--- a/llvm/lib/Target/ARM/ARMConstantIslandPass.cpp
+++ b/llvm/lib/Target/ARM/ARMConstantIslandPass.cpp
@@ -1324,7 +1324,8 @@ bool ARMConstantIslands::findAvailableWater(CPUser &U, unsigned UserOffset,
   MachineBasicBlock *UserBB = U.MI->getParent();
   BBInfoVector &BBInfo = BBUtils->getBBInfo();
   const Align CPEAlign = getCPEAlign(U.CPEMI);
-  unsigned MinNoSplitDisp = BBInfo[UserBB->getNumber()].postOffset(CPEAlign);
+  unsigned MinNoSplitDisp =
+      BBInfo[UserBB->getNumber()].postOffset(CPEAlign) - UserOffset;
   if (CloserWater && MinNoSplitDisp > U.getMaxDisp() / 2)
     return false;
   for (water_iterator IP = std::prev(WaterList.end()), B = WaterList.begin();;

[rengolin]

Adding some Arm folks, as constant island isn't as trivial as it sounds. Your description makes sense and the code could be interpreted as "someone forgot to subtract the start offset", but I want to make sure this is what we think it is.

Also, needs tests. I imagine a "large enough" function IR to trigger the split but a small enough BB IR to not be necessary, and show that there are no errors or crashes.

[davemgreen]

You can use the SPACE pseudo instruction in MIR to generate easier to read tests.

[statham-arm]

I understand that this change means that MinNoSplitDisp is now the exact offset between the user of the constant-pool entry and the place we're considering putting the entry itself. But why are we then splitting the block if that's more than half of U.getMaxDisp()? I would expect that if the distance we have to reach is actually larger than U.getMaxDisp(), that's when we need to split something.

But I don't understand why the previous code was comparing against U.getMaxDisp() / 2 either. Do you?

[pzhengqc]

I also have the same doubt actually. The check MinNoSplitDisp > U.getMaxDisp() / 2 is only relevant when CloserWater is true which means the pass has not converged after 15 iterations with the default arm-constant-island-max-iteration being 30. I guess the original intention was that, given that the pass has not converged with this many iterations (in practice, it rarely seems to take more than 3 iterations to converge), maybe it should change its strategy to be more aggressive now (therefore comparing against U.getMaxDisp() / 2 instead of U.getMaxDisp())?

[pzhengqc]

Adding some Arm folks, as constant island isn't as trivial as it sounds. Your description makes sense and the code could be interpreted as "someone forgot to subtract the start offset", but I want to make sure this is what we think it is.

Also, needs tests. I imagine a "large enough" function IR to trigger the split but a small enough BB IR to not be necessary, and show that there are no errors or crashes.

Thanks for the review, @rengolin! I think this hasn't been caught for so long because it's actually hard to trigger the check MinNoSplitDisp > U.getMaxDisp() / 2. The check is only performed when CloserWater is true which means the pass has not converged after 15 iterations with the default arm-constant-island-max-iteration being 30. In reality, it rarely seems to take more than 3 iterations for the pass to converge.

I've also added a test case to show that, with this fix, a small BB is not split even if it's far away from the beginning of the function.

[pzhengqc]

You can use the SPACE pseudo instruction in MIR to generate easier to read tests.

Thanks for the tip, @davemgreen!

[pzhengqc]

ping

[davemgreen]

It has been a while since I looked into the constant island pass, but this sounds OK. As you mention this should only kick in when we are already 15 iterations in, so is aiming to converge quickly at the expense of optimality. Did you run any checks (the llvm-test-suite or a boot strap), that can check we don't iterate for a long time now? Thanks.

[pzhengqc]

It has been a while since I looked into the constant island pass, but this sounds OK. As you mention this should only kick in when we are already 15 iterations in, so is aiming to converge quickly at the expense of optimality. Did you run any checks (the llvm-test-suite or a boot strap), that can check we don't iterate for a long time now? Thanks.

Yes, @davemgreen, I've run some checks (the llvm-test-suite, a boot strap, spec2000 and spec2006), but none of these tests seems to take more than 15 iterations for the constant island pass to converge. I guess, in reality, it's very rare for the check if (CloserWater && MinNoSplitDisp > U.getMaxDisp() / 2) to be true.

[davemgreen]

Sounds good to me then. Thanks for checking. LGTM