[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Generally looks great, thanks!



================
Comment at: lib/CodeGen/CGBlocks.cpp:923
+                     MakeAddrLValue(blockField, type,
+                     LValueBaseInfo(AlignmentSource::Decl, false)),
                      /*captured by init*/ false);
----------------
Please indent this line a little more to make it clear that it's part of the same argument.


================
Comment at: lib/CodeGen/CGExpr.cpp:884
+          if (BaseInfo)
+            BaseInfo->setAlignmentSource(Source);
         }
----------------
Places that fall back to the type of a pointer/reference/whatever should probably compute the entire BaseInfo from the type.  For example, if we wanted to properly support attribute may_alias on types, we'd need to compute it here.  So I think you should probably change getNaturalTypeAlignment and getNaturalPointeeTypeAlignment to produce an entire BaseInfo, even if you always fill it in with may_alias=false for now.

Here you'll also need to merge the BaseInfos of the original pointer and the new one.  You should probably have a method on LValueBaseInfo that does that, called maybe mergeForCast?


================
Comment at: lib/CodeGen/CGExpr.cpp:2256
             Address(CapLVal.getPointer(), getContext().getDeclAlign(VD)),
-            CapLVal.getType(), AlignmentSource::Decl);
+            CapLVal.getType(), LValueBaseInfo(AlignmentSource::Decl, MayAlias));
       }
----------------
Hmm.  I think your side of this is right, but I'm not sure that the original code is correct to override with AlignmentSource::Decl, because the captured field might be a reference.  In fact, I'm not sure why this is overwriting the alignment, either; it should be able to just return the result of EmitCapturedFieldLValue, and that should be more correct for the same reason.  Do any tests break if you do that?


Repository:
  rL LLVM

https://reviews.llvm.org/D33284