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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: lib/IR/ConstantFold.cpp:1081-1084
+          unsigned GVAlign =
+              GV->getParent()
+                  ? GV->getPointerAlignment(GV->getParent()->getDataLayout())
+                  : 0;
----------------
chandlerc wrote:
> craig.topper wrote:
> > chandlerc wrote:
> > > efriedma wrote:
> > > > 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.
> > > I don't think they're literally masking, but I think function pointers go down code paths that simplify heavily with alignment guarantees.
> > > 
> > > Target is x86. We're trying to confirm that this is actually what is happening.
> > > 
> > > Regarding the design points, I think a reasonable model we can use here: data layout could gave a field for function pointers, and it could take the following forms:
> > > a) is the function alignment (x86)
> > > b) is a known, fixed alignment of N, where N is in the datalayout string (AArch64, PPC)
> > > c) cannot be assumed due to potential use of LSB (ARM due to Thumb mode)
> > > 
> > > We could even make (c) be represented by a fixed alignment value of `1` and we could make (a) be represented by a fixed alignment value of `0`. This would match typical "special" alignment values in LLVM. And the duplication of a pointer's alignment for cases like PPC doesn't seem bad.
> > > 
> > > I feel like this would cover the vast majority of cases we're ever likely to care about with minimal complexity.
> > One thing we observed. This breaks optimization of the check for the virtual bit in member function pointers in the Itanium C++ ABI which set bit 0 if the member function is virtual. If we can see the creation of the function pointer and the use of the function pointer we could previously tell when it wasn't virtual because we could see it was just a function address which this code assumed was aligned and therefore bit 0 was 0.
> Is someone working on a fix? Can we put this behind a flag or something until everything is ready?
> 
> Currently, still have regressed workloads...
Here is a repro recipe that demonstrates the size increase (sorry for all the steps, I'll try to get it down to pure .ll later):

```
$ sudo apt install libprotobuf-dev protobuf-compiler
$ cat list.proto
syntax = "proto2";
message Value { optional string name = 1; }
message List { repeated Value values = 1; }
$ protoc list.proto --cpp_out=.
$ clang++ -c list.pb.cc -O2
```

The object file increases ~1% using clang built w/ this revision. Looking at some of the code that is optimized differently, they are dealing with function pointers, but AFAICT not literally masking them, so probably what Chandler suggested -- we're missing out on optimizations due to lack of alignment guarantees.


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

https://reviews.llvm.org/D55115