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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Thanks so much for the work here.

I landed a cleaned up version of this (just improving comments / formatting / test case layout) in r306379. For the sake of testing, I really wanted to get the range metadata to work as well, but it proved to be still quite broken. I've left a bunch of FIXMEs in that revision and described them in the commit log. If you're up for working on this, it would be really amazing. We should really be round tripping this kind of important information, especially for languages like Rust that leverage it heavily, but currently the propagation logic is really too weak to survive.

Thanks again, and sorry this whole thing got backed up for so long.
-Chandler



================
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`.
----------------
arielb1 wrote:
> 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?
No, this is correct.

If the bytes aren't part of the slice, they are undef. If they weren't undef, they would need to be part of the slice so we actually loaded those bytes.


https://reviews.llvm.org/D34285