In D159513#4645982, @yota9 wrote:

One more thing. It seems to be without force-patch the encodeValueAArch64 still would be called, but since functions would be moved to the new text there won't be negative address problem. I don't think there is a better way then using force-funcs so it would fail to patch entries & functions would stay on its current place, but please:

1. Please write the reason of using force-funcs option in the test, because after some time even I won't remember what I wrote above :)
2. Please add checks on llvm-bolt output that func1 and func2 were failed to patch and won't be optimised, since otherwise the test might pass, but the problem won't be checked. Alternatively (maybe even better) please check that in output binary the addresses are func1 < _start < func2. Maybe I would do both since the first option also shows more clearly why force-patch was used, but it's up to you.

Thanks!

update the test case.

1. check address order with llvm-nm --numeric-sort;
2. check the message of fail to patch entries;
3. add comment

In D159513#4645837, @Kepontry wrote:

Yes, you are absolutely right. This is my mistake, I didn't consider the sign bit when performing the right shift.

Thank you for your correction; I'm just getting started in this field, and I'll improve the quality of the patches I submit in the future.

Let's wait for the opinion from the Meta team.

Merged into test case bolt/test/AArch64/reloc-call26.s

LGTM, thanks for continuing this work.

gentle ping ...

Another way to avoid this mismatch is to set such blocks as 'Reachable' in CFIFixup, but it looks a bit different from the 'Unreachable block' case in CFIFixup to me and I think we still need a restore_state in the empty block. Any idea?

remove the incorrect description.

update the patch according to suggestions.

update the test case. This test case is generated after basic-block-sections pass and the name mapping between the basic block(bb.2.entry (align 256, bbsections 2)) and the label name(test.__part.2) from the assembly can be clearer in this way.

Hi @rahmanl, Sorry for the delay. I did not find any countercase for just checking the last terminator, and I spent some time investigating a pattern:

update according to comments

Hi @rahmanl, I tested with -O2, and the redundant branch can be removed by machine code sinking pass.

simplify code

Sounds fine. I tend to accept this patch since it is a stepping stone for the patch series, and we can revisit the constant expression case later after all patches merged.

tryToReplaceWithConstant method in SCCP does not update the lattice value map at SCCPSolver, and it might lead to a problem that

The reason why test cases crash at Solver.getLatticeValueFor is that SCCPSolver does not track the lattice value of a callbase argument. However, it is doable for SCCPSolver to track the lattice value of a ConstantExpr, and that is why I think skipping ConstantExpr is not an sound solution.

In D126355#3536597, @ChuanqiXu wrote:

We should generate a patch with option -U9999 to offer more context to review.

Thanks for working on this. Have you measured any performance/code size change?

Also, I feel we could do better since we've already mark it as overrefined in https://github.com/llvm/llvm-project/blob/cd2292ef824591cc34cc299910a3098545c840c7/llvm/lib/Transforms/Utils/SCCPSolver.cpp#L1254
So it looks possible to get the lattice successfully.

In D126355#3536597, @ChuanqiXu wrote:

We should generate a patch with option -U9999 to offer more context to review.

Thanks for the info and feedback. I have updated the diff file.