[PATCH] D25849: [GVN] Prevent load coercion with irregular vector types

[Davide Italiano via llvm-commits]

davide added inline comments.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:721
+  // going to crash later anyway.
+  if (llvm::alignTo(StoreSize, 8) != StoreSize)
+    return false;
----------------
reames wrote:
> StoreSize % 8 == 0 would seem to be a more standard way to express this check.
> 
> Also, checking only the StoreSize seems suspect.  Why do we not need to test the load size?
I think checking also the `LoadSize` is sensible.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:783
          "CanCoerceMustAliasedValueToLoad fail");
 
   // Convert source pointers to integers, which can be manipulated.
----------------
reames wrote:
> Have you looked at what's required to fix this code path?  This should be the only logic protected by the predicate you're modifying.
No, I didn't. Personally I looked at this because I got an internal report of a crash and I bothered to fix it.
The test is generated by a fuzzer so I'm not entirely sure how likely is to hit a situation like this in the wild, but still I thought it was worth fixing. I'm working with Daniel on NewGVN and I'd rather focus my efforts on that rather than fixing the load coercion system of some code I hope will go away in the foreseeable future. 


https://reviews.llvm.org/D25849