[PATCH] D73005: [Codegen] If reasonable, materialize clang's `AssumeAlignedAttr` as llvm's Alignment Attribute on call-site function return value

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 5 inline comments as done.
lebedev.ri added a comment.

Thank you for taking a look!



================
Comment at: clang/lib/CodeGen/CGCall.cpp:3836
+
+  llvm::Value *Alignment;                // May or may not be a constant.
+  llvm::ConstantInt *OffsetCI = nullptr; // Constant, hopefully zero.
----------------
erichkeane wrote:
> Does this need an initial value?  
I'd say no, we should always set it in constructor.
Although sure, let's add one.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:3841
+      : CGF(CGF_) {
+    if (!FuncDecl)
+      return;
----------------
erichkeane wrote:
> Does it make sense to call this on not a functiondecl?  Should this just be an assert?
This is modelling what's happening in `CodeGenFunction::EmitCall()`.
There, we clearly check that `TargetDecl` is not null,
which means i'd say we shouldn't assert that here, but also check.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:3852
+      return Attrs;
+    const auto *AlignmentCI = dyn_cast<llvm::ConstantInt>(Alignment);
+    if (!AlignmentCI)
----------------
erichkeane wrote:
> dyn_cast_or_null?
We shouldn't ever get null pointer there.
If that somehow happens (i'm not sure how), failed assert would be correct IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73005