[PATCH] D59730: [GVN] teach ConstantFolding correct handling of non-integral addrspace casts

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Analysis/ConstantFolding.cpp:96
 Constant *FoldBitCast(Constant *C, Type *DestTy, const DataLayout &DL) {
+  assert(CastInst::castIsValid(Instruction::BitCast, C, DestTy) &&
+         "Invalid constantexpr bitcast!");
----------------
Please separate and land this assert.


================
Comment at: lib/Analysis/ConstantFolding.cpp:332
+    uint64_t SrcSize = DL.getTypeSizeInBits(SrcTy);
+    if (SrcSize < DestSize)
+      return nullptr;
----------------
This may accidentally change semantics for struct typed loads, I'm not sure.


================
Comment at: lib/Analysis/ConstantFolding.cpp:543
+    if (Constant *Res = FoldReinterpretLoadFromConstPtr(C, MapTy, DL)) {
+      if (Res->isNullValue() && !LoadTy->isX86_MMXTy())
+        // Materializing a zero can be done trivially without a bitcast
----------------
I'm not following the need for this change.

Strong suggestion: Make the smallest possible change you can, and post that for review.  You've changed the patch here multiple times which is making it challenging to give you meaningful review across revisions.  Take the time to pull out helper functions if relevant, add asserts, whatever.  Just find a way to make the correctness of each step self documenting.  


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

https://reviews.llvm.org/D59730