This is an archive of the discontinued LLVM Phabricator instance.

Specify the access behaviour of the memcpy, memmove and memset intrinsics
AbandonedPublic

Authored by aadg on Feb 10 2013, 1:44 PM.

Details

Reviewers
None
Summary

memcpy, memmove : set volatile on the source or destination operand, not on the intrinsic itself.

This means that memcpy and memove will now have 2 parameters : isSrcVolatile and isDestVolatile in place of isVolatile. For now, the old 'isVolatile' parameter is duplicated to form the is(Src|Dest)Volatile parameters in order to
preserve the actual behaviour.

The isVolatile and setVolatile method are still there for compatibility reasons, but should disappear as soon as all users have moved to the new interface.

Those changes requires some clang tests to be updated accordingly (see http://llvm-reviews.chandlerc.com/D376)

The update of all IR tests to use the new format is moved to a separate patch, as it is quite big and mechanical.

No functional change. The code in LLVM or Clang using the isVolatile part will be updated in subsequent patches. It is expected to bring some improvement as the old behaviour prevented some optimizations to kick-in.

Diff Detail

Event Timeline

aadg updated this revision to Unknown Object (????).Feb 20 2013, 8:06 AM

Rebase + add a fix in AutoUpgrade : memcpy and memove do not return values, so they can not be named

aadg updated this revision to Unknown Object (????).Apr 15 2013, 6:17 AM

Rebase to tip of the day.

Sorry for the long delays, and thanks for reminding me about this. Comments below.

include/llvm/IR/IntrinsicInst.h
135–144

I would just sink this into the specific intrinsic classes so that we don't have to get the intrinsic ID and switch over it here.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4472–4474

Update to use the new APIs?

4510–4512

Here as well.

lib/Target/ARM/ARMFastISel.cpp
2292

This seems like a moderately horrible interface. I would much prefer to use the types to tell what to do than to count the volatile flags.

aadg added a comment.May 1 2013, 8:27 AM

I am fixing the patch right now and will resubmit it soon.

include/llvm/IR/IntrinsicInst.h
135–144

I aggree if we consider this method will stay --- but I think this method should just disappear as it has a dubious utility given the new interface. It is just kept with minimal changes for not breaking existing code and will be removed in a subsequent patch : all users of isVolatile have to take proper actions if the volatile attribute is set on src or dst.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
4472–4474

I adapted to the surrounding style, which does not use interface methods. But using them is definitely clearer. And cleaner. Will fix the patch.

4510–4512

Will do.

lib/Target/ARM/ARMFastISel.cpp
2292

Will fix it.

aadg updated this revision to Unknown Object (????).May 1 2013, 9:37 AM

Rebase + address Chandler's comments

The isVolatile method is not as clean as it could be, but knowing that it will disappear in the next commits, I do not think it is worth modifying the different classes to provide a nicer transient interface.

aadg updated this revision to Unknown Object (????).May 1 2013, 12:58 PM

Fix ARMFastISel/SelectCall calls.

I just found out I was not building the ARM target and did not caught this trivial error :(.

chandlerc resigned from this revision.Mar 29 2015, 10:59 AM
chandlerc removed a reviewer: chandlerc.

Sorry this got dropped.

If you're still interested in getting it in, please update the patch and re-add me as a reviewer and I'll take a look.

aadg abandoned this revision.Sep 7 2015, 3:11 AM