This is an archive of the discontinued LLVM Phabricator instance.

[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

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

-1 instead

1125

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

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

comments describing the members missing

1229

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

1379

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

1382

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

if (auto *DerefAA = ... )

1397

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

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

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

1804

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

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

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

1397

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

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

make it static

1386

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

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

1395

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.