[PATCH] D53490: [MIR] Provide a default alignment for stack objects

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: lib/CodeGen/MIRParser/MIRParser.cpp:574-578
+  // Try to get the default stack alignment from the IR function attrs.
+  if (F.hasFnAttribute(Attribute::StackAlignment))
+    return F.getFnStackAlignment();
+  else
+    return MF.getSubtarget().getFrameLowering()->getStackAlignment();
----------------
dsanders wrote:
> This isn't correct since both of these are the alignment of the stack frame itself, not a default alignment of the objects in the frame. You're right that alloca can choose an alignment when it isn't known (it's worth mentioning that this isn't true of other missing alignment information). However, the chosen alignment is necessarily a target decision. Aside from varying between targets, it can be also be context sensitive and varies between types and other things (e.g. fixed location stack frame objects generally don't get to chose, they're specified by the ABI).
> 
> Most importantly though, serializing+deserializing via MIR shouldn't change the alignment. With this code present, we would get different behaviour if we pass MIR from one pass to the next directly compared to if we write it to an intermediate file and read it back. I think we should fix AArch64 to account for missing alignment information instead.
Thanks. I agree this is not ideal. I believe none of the backends actually generate MIR with missing alignment information, I just triggered this when I forgot to include it.

I guess the right thing to do here is making the alignment non-optional on all stack objects?


https://reviews.llvm.org/D53490