[PATCH] D34285: [SROA] Be smarter when copying metadata to retyped loads

[Ariel Ben-Yehuda via Phabricator via llvm-commits]

arielb1 added inline comments.


================
Comment at: lib/Transforms/Scalar/SROA.cpp:2405-2409
+      // Any !nonnull metadata on the old load is also valid on the new load,
+      // so we can add it to the new load. This is valid even if there is a
+      // `zext` (which does not make much sense, but can occur if someone
+      // is treating pointers as integers) - if the loaded value is a 0,
+      // the extended value will also be `null`.
----------------
chandlerc wrote:
> I don't think the comment is quite correct...
> 
> The zext below doesn't matter for the metadata constraint. That zext is filling bytes of data that were read from past the end of an alloca and thus have no defined value. We can fill them with zext or not. The original load still cannot produce an integer value corresponding to the null pointer constant, and so the logic for mapping from pointer to integer remains valid.
> 
> The other issue is that while this adds the necessary test of the load type before copying nonnull metadata, it also teaches LLVM to form range metadata. I'll land this once I write up some test cases to make sure the range metadata is working correctly.
> We can fill them with zext or not. The original load still cannot produce an integer value corresponding to the null pointer constant, and so the logic for mapping from pointer to integer remains valid.

That's what this comment is saying. If the new (shortened) load could return a 0 that would invalidate the `!range` metadata, the original load would return a 0 plus N-1 undef bytes, which would invalidate the original `!nonnull` metadata. By contrapositive, replacing the `!nonnull` metadata with `!range` metadata is valid.

Actually I don't think that's correct - the other bytes are not guaranteed to be undef, only to be dead (right?). Should I add the zext check back?


https://reviews.llvm.org/D34285