[llvm] [GlobalOpt] Fix alignments of globals introduced for allocations (PR #216480)

[Ömer Sinan Ağacan via llvm-commits]

================
@@ -944,15 +944,37 @@ OptimizeGlobalAddressOfAllocation(GlobalVariable *GV, CallInst *CI,
       UndefValue::get(GlobalType), GV->getName() + ".body", nullptr,
       GV->getThreadLocalMode());
 
+  // Alignment of the return value of the allocator call.
+  Align GVAlign = CI->getPointerAlignment(DL);
+
+  // If the allocation function has a valid constant `allocalign` argument
+  // that's larger, increase the alignment.
+  const Value *AllocAlign = getAllocAlignment(CI, TLI);
+  if (AllocAlign) {
+    const ConstantInt *AllocAlignC = dyn_cast<ConstantInt>(AllocAlign);
+    if (AllocAlignC &&
+        AllocAlignC->getValue().ult(llvm::Value::MaximumAlignment)) {
+      uint64_t AllocAlignVal = AllocAlignC->getZExtValue();
+      if (llvm::isPowerOf2_64(AllocAlignVal)) {
+        GVAlign = std::max(GVAlign, Align(AllocAlignC->getAlignValue()));
----------------
osa1 wrote:

> I don't think we can just ignore the alignment if we don't understand what the code is doing

So the idea is that `allocalign` arguments normally don't influence the static information about the returned pointer as they don't need to be constants or even valid (i.e. power-of-two, within limits), so we could even ignore them completely here. That's what every other pass also needs to do as it's a dynamic value. (ignore if we can't make sense of the argument)

However in the limited cases of knowing the actual values (and they're valid), why not use them to increase the alignment? That's what the block is doing.

So it should be safe to ignore `allocalign`s completely and still do the transformation.

Thinking about this more as I type this: I think we should ignore it completely for this patch and consider dealing with it in a separate patch. I'll update to only look at the `call`s `align`.

> if we don't understand what the code is doing

But we do though. `getPointerAlignment` gives us the `align` in e.g. `call align 16 ptr @aligned_alloc(...)` and that gives the definitive answer: the pointer will be at least 16-byte aligned. The uncertain part is the `allocalign` part which we could even ignore entirely, but it's safe to make use of it when we know it to be more than the `call`'s `align`.

(I'll still update the patch to ignore the `allocalign` arguments, as mentioned above)

> I think the preconditions here ensure that we actually know every load and store instruction which accesses the allocation; instead of trying to derive the alignment of the allocation itself, we can take the maximum of the alignment of those instructions.

It's not trying to derive though, the allocation site gives us the definitive answer in the `call`'s `align`. Use sites need to load and store with the right `align`, otherwise it's UB (["It is the responsibility of the code emitter to ensure that the alignment information is correct"][1]).

We can still look at the use sites and if they all use the pointer with a larger alignment increase the global's alignment. I'm happy to look into this but let's do it separately as it's an optimization and I'm fixing a bug here and they don't need to go in the same patch.

[1]: https://llvm.org/docs/LangRef.html#id216


https://github.com/llvm/llvm-project/pull/216480