Page MenuHomePhabricator

[IR][Verifier] Allow IntToPtrInst to be !dereferenceable
ClosedPublic

Authored by rtaylor on Jul 18 2019, 3:38 PM.

Details

Summary

Allow IntToPtrInst to carry !dereferenceable metadata tag.
This is valid since !dereferenceable can be only be applied to
pointer type values.

Change-Id: If8a6e3c616f073d51eaff52ab74535c29ed497b4

Diff Detail

Repository
rL LLVM

Event Timeline

rtaylor created this revision.Jul 18 2019, 3:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 3:38 PM
arsenm added inline comments.Jul 18 2019, 4:47 PM
test/Verifier/dereferenceable-md.ll
88–90 ↗(On Diff #210691)

Can you also add a test for defeferencable_or_null

jdoerfert added inline comments.
test/Verifier/dereferenceable-md.ll
91 ↗(On Diff #210691)

And all other tests as well, e.g., a positive test (not here), wrong type, wrong number of operands, etc.

Also needs a langref update

rtaylor updated this revision to Diff 210900.Jul 19 2019, 2:31 PM

Added tests and updated LangRef

arsenm added inline comments.Jul 19 2019, 2:41 PM
test/Verifier/dereferenceable-md-inttoptr.ll
1–4 ↗(On Diff #210900)

Verifier tests should not run llc and cannot depend on a target. Most verifier just uses llvm-as

rtaylor updated this revision to Diff 210906.Jul 19 2019, 2:44 PM

Added !dereferenceable_or_null to syntax of inttoptr in LangRef

rtaylor updated this revision to Diff 210911.Jul 19 2019, 2:54 PM
rtaylor marked an inline comment as done.

Fixed test to use llvm-as and to just check for passing.

rtaylor marked an inline comment as done.Jul 20 2019, 10:36 AM

Are you planning to teach Value::getPointerDereferenceableBytes about this? The rest of the pipeline "should" then "just work" and use the information.

docs/LangRef.rst
9662 ↗(On Diff #210911)

Copy & Paste, these are no loads. Remove last sentence and add load metadata to the list of analogous things.

lib/IR/Verifier.cpp
3986 ↗(On Diff #210911)

Why ^? I personally find that confusing. || should do just fine.

rtaylor updated this revision to Diff 210969.Jul 20 2019, 12:30 PM

Changed LangRef.rst to reflect !dereferenceable is applicable to pointer types not
just load types.

Changed logic in verifier.

rtaylor updated this revision to Diff 210970.Jul 20 2019, 12:44 PM

Added IntToPtrInst to case statement in Value::getPointerDereferenceableBytes

I'm fine with this, let's give @arsenm a chance to look at it as well.

arsenm accepted this revision.Jul 21 2019, 11:14 AM

LGTM

This revision is now accepted and ready to land.Jul 21 2019, 11:14 AM

Thanks! Could you please also add a test to Analysis/ValueTracking/memory-dereferenceable.ll?

docs/LangRef.rst
8622 ↗(On Diff #210970)

Propose a change of the wording here to: This metadata can only be applied to certain instructions producing a pointer value.

After all, the metadata is not applied to the type itself. Same goes in the next paragraph.

rtaylor marked an inline comment as done.Jul 22 2019, 7:23 AM
rtaylor added inline comments.
docs/LangRef.rst
8622 ↗(On Diff #210970)

Are there certain instructions that produce a pointer value in which !dereferenceable cannot be applied? If so, then maybe we should list the instructions in the description(s) that it can be applied, for clarity? I'd rather not leave this as vague as "certain instructions" because the reader's first thoughts are going to be "which instructions?" Certainly if the description exists for every instruction than one could deduce it by why be vague if we don't have to?

rtaylor marked an inline comment as done.Jul 22 2019, 7:32 AM
rtaylor added inline comments.
docs/LangRef.rst
8622 ↗(On Diff #210970)

Also, the current Parameter Attributes reads quite simliar: This attribute may only be applied to pointer typed parameters.

So in this case, it's actually attached to the type and not the instruction?

arsenm added inline comments.Jul 22 2019, 7:33 AM
docs/LangRef.rst
8622 ↗(On Diff #210970)

Calls aren't allowed because they have matching attributes that are superior to the metadata. Other cases I think are less clear and may make sense to allow it in the future.

I think this section should be split out. As is, you are just updating the notes on metadata allowed for loads. The load and inttoptr can then reference the !dereferencable section

rtaylor marked an inline comment as done.Jul 22 2019, 7:54 AM
rtaylor added inline comments.
docs/LangRef.rst
8622 ↗(On Diff #210970)

Agree. I think these should probably have their own '`....`' Metadata section?

rtaylor updated this revision to Diff 211112.Jul 22 2019, 8:41 AM

Changed LangRef.rst: Moved !dereferenceable and !dereferenceable_or_null to their
own section in Metadata and pointed inttoptr and load to that section.

Added test for Analysis/ValueTracking.

nhaehnle added inline comments.Jul 22 2019, 8:57 AM
docs/LangRef.rst
5306–5324 ↗(On Diff #211112)

Could you add the incantations for hyperlinks? ".. _blahblah" for the anchor and :ref:.... <blahblah> for the link.

rtaylor updated this revision to Diff 211122.Jul 22 2019, 9:20 AM

Added hyperlinks to LangRef.rst

nhaehnle accepted this revision.Jul 23 2019, 2:35 AM

LGTM, though I believe the hyperlinks in :ref: need to be without the underscore (and other examples of :ref: use seem to corroborate that).

rtaylor updated this revision to Diff 211282.Tue, Jul 23, 6:40 AM

Removed preceding underline to ref hyperlink.

rtaylor updated this revision to Diff 211283.Tue, Jul 23, 6:44 AM

Remove underline to ref hyperlink from invariant.group

This revision was automatically updated to reflect the committed changes.