Page MenuHomePhabricator

[SLC] Dereferenceable annonation - handle valid null pointers
ClosedPublic

Authored by xbolva00 on Aug 13 2019, 1:09 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Aug 13 2019, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 1:09 PM
xbolva00 planned changes to this revision.Aug 13 2019, 1:57 PM
xbolva00 updated this revision to Diff 214911.Aug 13 2019, 1:59 PM

I'm fine with this, @reames ?

I'm fine with this, @reames ?

Definitely not. I've never seen that utility on Function, but whatever it's supposed to do, it completely ignores address spaces (our major source of defined nulls) while doing it.

A much simpler surgical fix would be to replace the use of getDereferenceableOrNullBytes with getDereferenceableBytes. This would eliminate the "or null concern" from the merge logic.

Johannes, I didn't take time to fully understand your concern, but if it's broader than the above, please revert the original patch and handle in normal review.

I'm fine with this, @reames ?

Definitely not. I've never seen that utility on Function, but whatever it's supposed to do, it completely ignores address spaces (our major source of defined nulls) while doing it.

The utility function can take a address space and it is used in other places too. To give it the address spaces of the argument pointers would be an option, but ...

A much simpler surgical fix would be to replace the use of getDereferenceableOrNullBytes with getDereferenceableBytes. This would eliminate the "or null concern" from the merge logic.

Johannes, I didn't take time to fully understand your concern, but if it's broader than the above, please revert the original patch and handle in normal review.

... we can, as @reames noted, not change the _or_null part at all and only work on the deref(X) parts.

xbolva00 updated this revision to Diff 214935.Aug 13 2019, 3:02 PM

Handle addrspaces.

...
Johannes, I didn't take time to fully understand your concern, but if it's broader than the above, please revert the original patch and handle in normal review.

Hey, read back over this and realized my tone rather snappy here. Sorry about that, was annoyed at something else at the time I wrote this and it came through more than I realized. Sorry.

xbolva00 updated this revision to Diff 214938.Aug 13 2019, 3:06 PM

I think this should be safe now.

I think this should be safe now.

if (!F->nullPointerIsDefined(AS)) is what I was expecting.

And a test is missing.

nullPointerIsDefined(F, AS) .. cool :)

Test is in extra file due to memcmp redefinition issue.

jdoerfert added inline comments.Aug 13 2019, 3:47 PM
lib/Transforms/Utils/SimplifyLibCalls.cpp
206 ↗(On Diff #214938)

Now, one could just keep this annotation if NullPointerIsDefined(F, AS) was true.

test/Transforms/InstCombine/mem-deref-bytes-addrspaces.ll
6 ↗(On Diff #214938)

The point was, "null-pointer-is-valid"="true" should not be needed if the AS is set to (sth not known to have an invalid null).

xbolva00 updated this revision to Diff 215067.Aug 14 2019, 3:11 AM

Tests.
Addressed review note.

xbolva00 updated this revision to Diff 215071.Aug 14 2019, 3:17 AM
xbolva00 updated this revision to Diff 215073.Aug 14 2019, 3:23 AM
xbolva00 marked an inline comment as done.
xbolva00 marked an inline comment as done.

I like it. We don't loose information and it should be always correct. @reames, still concerned?

I have no problem with this patch (i.e. it can land if Johannes approves)

A conceptual point worth mentioning is that seems to essentially be inferring non-null for the intrinsics in question. We have a nonnull attribute on params and nonnull + deref_or_null should already be converted to deref. (If not, we should fix that.) It might be worth thinking about how to leverage the more generic code.

xbolva00 added a comment.EditedAug 14 2019, 9:50 AM

I have no problem with this patch (i.e. it can land if Johannes approves)

A conceptual point worth mentioning is that seems to essentially be inferring non-null for the intrinsics in question. We have a nonnull attribute on params and nonnull + deref_or_null should already be converted to deref. (If not, we should fix that.) It might be worth thinking about how to leverage the more generic code.

Next step (https://reviews.llvm.org/D53342) :) As noted in D53342 we need to be careful about adding nonnull.

jdoerfert accepted this revision.Aug 14 2019, 10:08 AM

I have no problem with this patch (i.e. it can land if Johannes approves)

LGTM.

A conceptual point worth mentioning is that seems to essentially be inferring non-null for the intrinsics in question. We have a nonnull attribute on params and nonnull + deref_or_null should already be converted to deref. (If not, we should fix that.) It might be worth thinking about how to leverage the more generic code.

The Attributor does this already but there are a few points where we need to improve it.
I think these patches should encode "known" information and the Attributor (run early) will combine whatever it can.

This revision is now accepted and ready to land.Aug 14 2019, 10:08 AM
This revision was automatically updated to reflect the committed changes.