[PATCH] D55115: [IR] Don't assume all functions are 4 byte aligned

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/IR/ConstantFold.cpp:1081-1084
+          unsigned GVAlign =
+              GV->getParent()
+                  ? GV->getPointerAlignment(GV->getParent()->getDataLayout())
+                  : 0;
----------------
chandlerc wrote:
> This is actually a pretty significant regression for platforms where functions are aligned.
> 
> The implementation of `getPointerAlignment` doesn't do any checks and always returns zero for functions. While this may be necessary on some platforms, it seems bad to just do this blindly. In particular, it doesn't even look at explicit alignment in that case, making this a significant and surprising regression in that case.
> 
> While I generally support fixing this, I think we need to enhance `getPointerAlignment` to actually return something useful if we're going to delegate to it here. This may require putting some of the underlying alignment constraints into the datalayout (such as the thumb-mode LSB stashing).
> 
> Last but not least, I don't understand why this code can or should support global variables that are not in a module, and then constant fold them differently. That seems like a bad precedent to set. Maybe we should assert that there is a module here and fix the code that fails to put its globals into a module?
I've never seen code that computes "FnPtr & -4", but I guess you must have some somewhere. (We fixed getPointerAlignment for all the other places a while back, and nobody noticed.)  Which target are you seeing the size regression on?  Are you sure LLVM was actually computing the alignment correctly for your code?

In general, there are two aspects to function pointer alignment: one, whether an arbitrary function pointer has known alignment, and two, whether we can compute better alignment by examining a specific function.  For the first, there are some targets where the alignment is known: AArch64 function pointers point to an instruction which is four-byte-aligned, PowerPC function pointers point to a descriptor which is pointer-aligned, etc.  For the second, we can often do better: x86 function pointers always have the alignment of the function, ARM function pointers have the alignment of the function in ARM mode, etc.

I'm not sure how much of this makes sense to put into the datalayout.  We could add a "function pointer alignment" integer for the first.  We could also add a boolean "does the alignment of a function always dictate the pointer alignment" bit for the second.  If we want to handle targets like ARM, though, we probably want to move the folding elsewhere and use a target hook, instead of trying to put arbitrary predicates into the datalayout.


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

https://reviews.llvm.org/D55115