[GlobalOpt] Reduction of global aggregate with alignment constraint unnecessarily propagates alignment to some elements

[brunodf-snps]

When GlobalOpt performs SRA on a global aggregate with an alignment constraint, it seems to propagate the alignment to some of the elements haphazardly. For example:

alignas(32) static int params[8];

On a plain 32 bit architecture, the array elements respectively have alignment: 32, 4, 8, 4, 16, 4, 8, 4
GlobalOpt SRA creates individual global variables for the elements with respective alignments: 32, 32, 8, 32, 16, 32, 8, 32

I don't see any benefit to increase the alignments elements 1, 3, 5, 7 from 4 to 32. This is wasteful because the new elements cannot be mapped consecutively in memory anymore.

Godbolt demo: https://godbolt.org/z/WjbPjY9Kh

[brunodf-snps]

In my debugging of GlobalOpt, there seems to be some logic flaw in this part:

llvm-project/llvm/lib/Transforms/IPO/GlobalOpt.cpp

Lines 584 to 586 in 2d7f34f

	Align NewAlign = commonAlignment(StartAlignment, OffsetForTy);
	if (NewAlign > DL.getABITypeAlign(Ty))
	NGV->setAlignment(NewAlign);

Some lines above NGV has copied all attributes from GV. This includes the alignment. When the calculated NewAlign is not better than the ABI type align, this code seems to retain the original aggregate alignment on the element unintentionally.

@nikic I notice that your commit 4796b4a increases the expected alignment values in some of the tests such as llvm/test/Transforms/GlobalOpt/globalsra-align.ll. Is that intentional?

[nikic]

Agree with your analysis. We should be unconditionally setting to something like std::max(NewAlign, DL.getABITypeAlign(Ty)) I think?

[brunodf-snps]

Agree with your analysis. We should be unconditionally setting to something like std::max(NewAlign, DL.getABITypeAlign(Ty)) I think?

Yes, thanks for confirming. I'll try to create a pull request. It will have to update the expected alignments in some of the tests I think.