Page MenuHomePhabricator

[SROA] Be smarter when copying metadata to retyped loads
Needs ReviewPublic

Authored by arielb1 on Jun 16 2017, 11:13 AM.

Details

Summary

Use the same code as InstCombine when copying metadata to a retyped load. This makes sure that !nonnull metadata is turned into the equivalent !range metadata when a pointer load is retyped to an integer load (PR32902).

Diff Detail

Event Timeline

arielb1 created this revision.Jun 16 2017, 11:13 AM

This is a hard blocker for Rust LLVM 5.0, and is also blocking us from backporting a perf patch.

dberlin added inline comments.
include/llvm/Transforms/Utils/Local.h
383 ↗(On Diff #102855)

I have no idea what this comment is trying to say ;)

385 ↗(On Diff #102855)

By value do you mean "result"?

IE the result of the load?

If so, please use that, as it's much clearer.
Loads don't have values, they have addresses, and results.

chandlerc edited edge metadata.Jun 25 2017, 7:41 PM

FYI, I started looking at this. I think a somewhat substantially different refactoring of the instcombine API makes this much simpler by *only* extracting the logic for nonnull and range propagation rather than extracting all of it.

I've almost got the refactoring done, and I'm happy to land the rest for you rather than making you go through a bunch of iterations of code review.

I landed some simpler refactorings in r306267 that should be enough to implement this.

However, I noticed that SROA actually assert fails on your test case without this patch, and in a seemingly unrelated manner. I want to understand this before landing your patch (or equivalent using different API). That said, feel free to update your patch based on r306267 or I will when I finish debugging (or if you can debug the assert, let me know).

I definitely want to get this unblocked for Rust, but it seems pretty scary to paper over an assert failure. Anyways, will try to keep this moving.

The assert failure is exactly the reason I wrote this patch. Without
this patch, the !nonnull metadata is copied from the %bar* load to
the i64 load, which triggers the assertion.

By value do you mean "result"?
IE the result of the load?

If that's the LLVM term, yeah.

arielb1 updated this revision to Diff 103918.Jun 26 2017, 3:40 AM

More conservative version on top of r306267.

chandlerc added inline comments.Jun 26 2017, 1:13 PM
lib/Transforms/Scalar/SROA.cpp
2405–2409

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.

arielb1 added inline comments.Jun 26 2017, 3:01 PM
lib/Transforms/Scalar/SROA.cpp
2405–2409

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?

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

lib/Transforms/Scalar/SROA.cpp
2405–2409

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.

nagisa resigned from this revision.Aug 29 2020, 8:07 AM
luqmana resigned from this revision.Oct 6 2020, 12:13 AM