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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

We've noticed significant regressions on x86 in code size that seem likely root caused to this going in... see my comments below. I think this has caused a regression on most targets. =[



================
Comment at: lib/IR/ConstantFold.cpp:1081-1084
+          unsigned GVAlign =
+              GV->getParent()
+                  ? GV->getPointerAlignment(GV->getParent()->getDataLayout())
+                  : 0;
----------------
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?


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

https://reviews.llvm.org/D55115