Page MenuHomePhabricator

[Attributor] Deduce "dereferenceable" attribute
ClosedPublic

Authored by uenoku on Jul 17 2019, 11:38 AM.

Details

Summary

Deduce dereferenceable attribute in Attributor.

These will be added in a later patch.

  • dereferenceable(_or_null)_globally (D61652)
  • Deduction based on load instruction (similar to D64258)

Diff Detail

Repository
rL LLVM

Event Timeline

uenoku created this revision.Jul 17 2019, 11:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 11:38 AM
uenoku updated this revision to Diff 210385.Jul 17 2019, 12:00 PM

Fix typo.

uenoku updated this revision to Diff 210408.Jul 17 2019, 1:57 PM

Refactor initialize.

uenoku updated this revision to Diff 210414.Jul 17 2019, 2:18 PM

Fix patch.

Initial comments.

llvm/lib/Transforms/IPO/Attributor.cpp
379 ↗(On Diff #210414)

-1 instead

1125 ↗(On Diff #210414)

Do we make sure dereferenceable(0) is invalid IR? If not, we should. (The logic you use above relies on it and other places might be as well.)

1148 ↗(On Diff #210414)

This can be commited separately with the other nonnull changes above. Please change the order in this conditional to first check the attributes and then call isKnownNonZero, or better, remove the attribute checks and make sure isKnownNonZero performs them.

1226 ↗(On Diff #210414)

comments describing the members missing

1229 ↗(On Diff #210414)

While it practically doesn't make a difference (I think) I would have suspected both states to be asked here.

1379 ↗(On Diff #210414)

Add a TODO here wrt. tacking the globally flag as well, or just do it.

1382 ↗(On Diff #210414)

Here and below it might make sense to move the declaration into the conditional:

if (auto *DerefAA = ... )

1397 ↗(On Diff #210414)

If Offset is not 0, Base cannot be null (due to the inbounds properties). I thought about relying on the attribute to know this but that is actually not always possible. Instead, I think you can here say something like:
Offset != 0 || BaseDerefAA->isAssumedNonNull()

Furthermore, we could strip non-inbounds offsets as well, I think. Thought, we then have to be more careful with the difference calculation, e.g., only non-negative Offset values would then be allowed.

What do you think?

1410 ↗(On Diff #210414)

true should be !NullPointerIsDefined(&getAnchorScope(), V.getType()->getPointerAddressSpace()) to allow nullptr as valid pointer.

Shouldn't one of the two checks above subsume the other?

1467 ↗(On Diff #210414)

Why not call computeAssumedDerefenceableBytes on the CS.getArgOperand(ArgNo) value?

1804 ↗(On Diff #210414)

Whitelist check missing here and above (and in existing code).

uenoku updated this revision to Diff 210591.Jul 18 2019, 9:04 AM
uenoku marked 12 inline comments as done.

Address comments.

I'll split a patch for nonnull initialization and fix whitelist in a few days.

llvm/lib/Transforms/IPO/Attributor.cpp
1148 ↗(On Diff #210414)

or better, remove the attribute checks and make sure isKnownNonZero performs them.

I think isKnownNonZero would not check parameter attribute in call site so it is not correct.

1229 ↗(On Diff #210414)

Even though NonNullGlobalState was not valid, we can deduce dereferenceable_or_null so I think this is appropriate.

1397 ↗(On Diff #210414)

Furthermore, we could strip non-inbounds offsets as well, I think. Thought, we then have to be more careful with the difference calculation, e.g., only non-negative Offset values would then be allowed.

I'll try it later.

1467 ↗(On Diff #210414)

It is already called below.

Can you make sure this passes the llvm test suite when the Attributor is turned on?

llvm/lib/Transforms/IPO/Attributor.cpp
1377 ↗(On Diff #210591)

make it static

1386 ↗(On Diff #210591)

and the nonnull flag. Maybe add a parameter bool &IsNonNull and bool &IsGlobal.

uenoku updated this revision to Diff 210692.Jul 18 2019, 3:46 PM
uenoku marked 2 inline comments as done.

Address comments.

jdoerfert accepted this revision.Jul 21 2019, 10:26 AM

This LGTM assuming you tried it on the test suite. Clang format the patch please and see the two comments below.

llvm/lib/Transforms/IPO/Attributor.cpp
1229 ↗(On Diff #210692)

Change the name to something meaningful or remove it, should not be necessary anyway.

1395 ↗(On Diff #210692)

if you do not track them, set them to false. I think tracking nonnull should be easy, you compute it below almost in all cases anyway.

This revision is now accepted and ready to land.Jul 21 2019, 10:26 AM
uenoku updated this revision to Diff 211015.Jul 21 2019, 4:24 PM

Address comments and add stats. test-suite passed. However as far as I see it, no new dereferenceable was deduced.

uenoku marked an inline comment as done.Jul 21 2019, 4:24 PM

Address comments and add stats. test-suite passed. However as far as I see it, no new dereferenceable was deduced.

if (!Attr.isEnumAttribute())
  return;

Will block the attribute to reach stats. Could you re-run (with --cmake-define=TEST_SUITE_COLLECT_STATS=ON)?

uenoku added a comment.EditedJul 22 2019, 11:42 PM

Will block the attribute to reach stats. Could you re-run (with --cmake-define=TEST_SUITE_COLLECT_STATS=ON)?

Oh, I see. All test passed and here is a summary:

attributor.NumCSArgumentDereferenceable 79604.0
attributor.NumFnArgumentDereferenceable 1105.0
attributor.NumFnReturnedDereferenceable 123.0
uenoku updated this revision to Diff 211258.Jul 23 2019, 12:53 AM

Minor update.

This revision was automatically updated to reflect the committed changes.