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

[Jameson Nash via Phabricator via llvm-commits]

vtjnash marked 2 inline comments as done.
vtjnash added inline comments.


================
Comment at: lib/Analysis/ConstantFolding.cpp:332
+    uint64_t SrcSize = DL.getTypeSizeInBits(SrcTy);
+    if (SrcSize < DestSize)
+      return nullptr;
----------------
reames wrote:
> This may accidentally change semantics for struct typed loads, I'm not sure.
I don't see how that could happen: the while loop below repeated slices up `C` (aka SrcTy, aka SrcSize) until SrcSize==DestSize. If `SrcSize < DestSize`, then transitivity ensures that was never true. That should make sense too from a desired behavior standpoint, since if we're casting a small constant to a larger one, we're appending unknown (not necessarily undef) bits.

Since we need to catch the "isNullValue" obvious cases below to avoid regressing capabilities (and failing the GVN tests) while introducing the `DL.isNonIntegralPointerType` correctness check, we need this early-exit first to check the same size precondition that would get implied by the while loop.


================
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
----------------
reames wrote:
> 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.  
It's the bug you mentioned above (this line ... violates the specification of the function). I've split off this fix into https://reviews.llvm.org/D65057.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59730