[PATCH] D36841: [SimplifyCFG] Fix for PR34219: Preserve alignment after merging conditional stores.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2969
   PStore->getAAMetadata(AAMD, /*Merge=*/false);
   PStore->getAAMetadata(AAMD, /*Merge=*/true);
   SI->setAAMetadata(AAMD);
----------------
efriedma wrote:
> This looks weird; why are we calling "PStore->getAAMetadata" twice?
No comment on the getAAMetadata() calls?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2976
+    // Choose the first non-zero alignment, if any.
+    SI->setAlignment(MaxAlignment);
 
----------------
This isn't really handling zero alignment correctly; to get the right number here, you have to convert to the actual alignment using datalayout.  (I'm planning to write a patch soon to get rid of this whole zero alignment crap; it's unnecessary given that datalayout is now mandatory, and it's bitten us too many times.)

It would be nice to have a comment briefly explaining why we pick the minimum.


https://reviews.llvm.org/D36841