Page MenuHomePhabricator

[IR] ArgMemOnly functions with WriteOnly ptr args do not read memory.
Needs RevisionPublic

Authored by fhahn on Jan 13 2020, 4:41 PM.

Details

Summary

Calls/functions with ArgMemOnly only access memory via pointer-type
arguments. If all pointer-type arguments are WriteOnly, this means that
the function does not read memory.

This is relevant, for example, for the current definition of
@llvm.memset, which is marked as ArgMemOnly with WriteOnly pointers arguments.
Without this patch, doesNotReadMemory returns false for @llvm.memset.

I am not sure where the best place to add tests for this would be?

This impacts a single attributor test, which uses the following
declaration

declare <4 x i32> @test11_1(<4 x i32*>) argmemonly nounwind readonly

With the patch, this function will be treated as readnone. I think that
is fine according to the ArgMemOnly definition: there are no
pointer-typed arguments (the only argument is a vector of pointers), so
no memory can be accessed.

If 'pointer-typed arguments' could be interpreted as any argument containing a
pointer, this patch would have to be more careful with checking the arguments.

Diff Detail

Event Timeline

fhahn created this revision.Jan 13 2020, 4:41 PM
Herald added a project: Restricted Project. · View Herald Transcript

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

Unit tests: pass. 61772 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

We could "deduce" it, based on this logic, and then add writeonly. That would work for non intrinsics as well.

fhahn added a comment.Jan 14 2020, 3:45 PM

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

Yes we should probably just mark llvm.memset as IntrWriteMem.

We could "deduce" it, based on this logic, and then add writeonly. That would work for non intrinsics as well.

Yep I think it might be beneficial to use the logic in the patch to set the write-only function attribute at the time attributes are added to a call/function. I am not sure how common this is (argmemonly + writeOnly/readOnly argument attributes), but that it was missing for an intrinsic like memset seems to indicate how easy things like that can be missed. Alternatively we could use the logic to warn/fail on missing attributes. WDYT?

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

Yes we should probably just mark llvm.memset as IntrWriteMem.

Agreed.

We could "deduce" it, based on this logic, and then add writeonly. That would work for non intrinsics as well.

Yep I think it might be beneficial to use the logic in the patch to set the write-only function attribute at the time attributes are added to a call/function. I am not sure how common this is (argmemonly + writeOnly/readOnly argument attributes), but that it was missing for an intrinsic like memset seems to indicate how easy things like that can be missed. Alternatively we could use the logic to warn/fail on missing attributes. WDYT?

I dislike the warn/fail idea and I would still argue for deduction instead:

  1. We will hopefully soon have a way for users to add attributes like argmemonly and writeonly, so if they miss one we can just infer it.
  2. It is also a "shortcut" if we can infer it if after partial code specialization eliminated the read arguments or the non-argument accesses.

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

Yes we should probably just mark llvm.memset as IntrWriteMem.

Agreed.

Posted D72789.

We could "deduce" it, based on this logic, and then add writeonly. That would work for non intrinsics as well.

Yep I think it might be beneficial to use the logic in the patch to set the write-only function attribute at the time attributes are added to a call/function. I am not sure how common this is (argmemonly + writeOnly/readOnly argument attributes), but that it was missing for an intrinsic like memset seems to indicate how easy things like that can be missed. Alternatively we could use the logic to warn/fail on missing attributes. WDYT?

I dislike the warn/fail idea and I would still argue for deduction instead:

  1. We will hopefully soon have a way for users to add attributes like argmemonly and writeonly, so if they miss one we can just infer it.
  2. It is also a "shortcut" if we can infer it if after partial code specialization eliminated the read arguments or the non-argument accesses.

I'll check how to best do it at call/declare construction time.

reames requested changes to this revision.Feb 25 2020, 10:08 AM

This should definitely be an inference rule. See FunctionAttrs.

This revision now requires changes to proceed.Feb 25 2020, 10:08 AM