Page MenuHomePhabricator

Introduce !value.align metadata for load instruction
ClosedPublic

Authored by apilipenko on Sep 14 2015, 11:03 AM.

Details

Summary

Since D9791 we take alignment into account to decide if it's safe to speculate load. Now pointer has to be both dereferenceable and properly aligned to be speculative loadable. It means that we want to support align attribute everywhere we support dereferenceable attribute.

Introduce new !value.align metadata for load instruction. This metadata specifies alignment of the loaded pointer value.

Diff Detail

Repository
rL LLVM

Event Timeline

apilipenko retitled this revision from to Introduce !value.align metadata for load instruction.
apilipenko updated this object.
apilipenko added reviewers: hfinkel, reames, sanjoy.
apilipenko added a subscriber: llvm-commits.
sanjoy added inline comments.Sep 15 2015, 9:32 PM
docs/LangRef.rst
6856 ↗(On Diff #34706)

Is it restricted to be a power of two (I'd guess so)? If so, that should be noted here.

lib/Analysis/ValueTracking.cpp
2962 ↗(On Diff #34706)

With your changes, does this bit remain necessary? I think this part is unsound anyway (since it relies on differentiating different pointer types of the same address space).

apilipenko updated this revision to Diff 34895.Sep 16 2015, 8:43 AM

Add a sentence to LangRef to require alignment to be a power of 2.

lib/Analysis/ValueTracking.cpp
2962 ↗(On Diff #34706)

I'm going to remove this in a separate patch.

hfinkel edited edge metadata.Sep 22 2015, 2:59 PM

I assume that you want to call this '!value.align' to differentiate it from the existing 'align' argument to the load. This is syntactically unnecessary, and while I agree that it could seem confusing, is also inconsistent with the other similar metadata. We don't have value.nonnull or value.dereferenceable, and I'm inclined to think that we should just call this !align for consistency.

apilipenko edited edge metadata.

Rename !value.align to !align.

hfinkel accepted this revision.Sep 25 2015, 3:00 PM
hfinkel edited edge metadata.

This LGTM; I agree this is a common case and worth representing using metadata instead of calls to @llvm.assume (just as with !nonnull). However, as with !nonnull, if we add this metadata, the associated follow-up work needs to be done:

  1. Add code that canonicalizes @llvm.assume calls to this metadata when possible.
  2. Audit all places where we combine and/or drop metadata on loads, and preserve / combine this metadata when possible.

If you'll do those things as follow-up, then go ahead.

This revision is now accepted and ready to land.Sep 25 2015, 3:00 PM
This revision was automatically updated to reflect the committed changes.