[PATCH] D127220: [BOLT][AArch64] Preserve in text object alignment

[Vladislav Khmelevsky via Phabricator via llvm-commits]

yota9 marked an inline comment as done.
yota9 added inline comments.


================
Comment at: bolt/include/bolt/Core/BinaryFunction.h:1104
+  // Return original alignment value of the function
+  uint16_t getInputAlignment() const { return 1 << (ffsll(getAddress()) - 1); }
+
----------------
rafauler wrote:
> In general in BOLT we have no way of recovering the correct original alignment. I'm afraid having a function that returns this information might be misleading to users of this API, I'm more comfortable with a function named "guessInputAlignment" than one that says "getInputAlignment".
I agree, thanks


================
Comment at: bolt/lib/Passes/Aligner.cpp:172-173
     if (!BF.size() && BF.hasIslandsInfo()) {
-      const uint16_t Alignment = BF.getConstantIslandAlignment();
+      const uint16_t Alignment = BF.getInputAlignment();
+      BF.setConstantIslandAlignment(Alignment);
       if (BF.getAlignment() < Alignment)
----------------
rafauler wrote:
> I don't completely follow this part and perhaps we can clarify: why are we aligning a constant island against the presumed alignment of its parent function, and not against the alignment of the constant island itself in the input?
> 
> Another thing that could be an issue here is if the function happens to be at an arbitrary round address (such as 0x100000) and now we have to emit the island with MBs worth of alignment because we have no clue what is the correct alignment. That's bound to happen if we are processing the very first function in the .text section and if it has a constant island, since that first function will probably be aligned to a page boundary, which may push the constant island beyond the reach of some instructions. That's why in general I think that's a fragile strategy, but if we have to do it and if we are resorting to guessing the correct alignment, maybe we should put a cap on it? But before proceeding with this diff, I have one alternative suggestion below.
> 
> I read the offending code in openssl. Can we try -skip-funcs=problematic_function_name and just skip supporting it (I'm not sure if that works with AArch64, though). I think BOLT is not in a position of supporting arbitrary assembly code, and code that makes assumptions on the layout of functions is bound to break. This hits AArch64 more strongly because it is okay to have data in code for AArch64, and some programmers like making assumptions on the layout of the data. That's why it's easier for us to provide support for languages such as C++, which has a standard that says that doing pointer arithmetic with function/object pointers is undefined behavior.
> 
> 
>> why are we aligning a constant island against the presumed alignment of its parent function

This is under !BF.size() if, another words if it is object in code handled as empty function by BOLT.

>> Another thing that could be an issue here is if the function happens to be at an arbitrary round address (such as 0x100000) and now we have to emit the island with MBs worth of alignment because we have no clue what is the correct alignment.

I agree, that it might be the problem. How about to add smth like AlignCIMaxBytes option, equal to 512 by default? If the alignment of CI is higher the warning would be displayed. 

>> Can we try -skip-funcs=problematic_function_name and just skip supporting it

It might work, but ideally we would like to process whole objects from the original text. E.g. we've got beta option to remove old text section from the binary, so skip funcs is not an option in that case. 

I agree that these things are kind of hacky, but it looks like in this case we can handle the majority of the cases, I assume it would be nice to have such functionality.. For now I will try to add the option above and re upload the review. 

Thank you for your comments!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127220