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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a subscriber: hfinkel.
efriedma added a comment.

Chandler, when you have a chance, can you look at the LangRef changes, since you put some thought into the design?

I think the DataLayout/LangRef changes look correct.

I agree it isn't necessary to fix every target in the initial patch; it would take a long time to get review, and no one person can properly test every target.  But I'm afraid we'll forget to fix some target if someone doesn't go through and post followup patches for every target.  (It doesn't really even matter if the initial patches are right, as long as there's something for the target maintainers to look at.)



================
Comment at: llvm/include/llvm/IR/ConstantFold.h:7
+//
+//===----------------------------------------------------------------------===//
+//
----------------
Are you just moving this for the unittests?  You shouldn't need to; you can just check the result of `ConstantExpr::get(Instruction::And, ...`


================
Comment at: llvm/lib/IR/ConstantFold.cpp:1087
+          if (GVAlign == 0U && isa<Function>(GV))
+            GVAlign = 4U;
 
----------------
michaelplatings wrote:
> efriedma wrote:
> > Using "4" as a default is dangerous; on x86, the alignment of the pointer corresponds to the alignment of the function, but might be less than 4 if it isn't explicitly specified.
> I've put in a comment to say as much. Sadly I'm not in a position to change this behaviour without causing a code size regression.
>From the discussion on D55115, adding the right default to x86 (Fn8) is probably enough to avoid the regression, but I guess we can deal with that as a followup.

Please add a comment like `// FIXME: This code should be deleted once existing targets have appropriate defaults`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57335