This is an archive of the discontinued LLVM Phabricator instance.

Invariant start/end intrinsics overloaded for address space
ClosedPublic

Authored by anna on Jul 19 2016, 8:35 AM.

Details

Summary

The llvm.invariant.start and llvm.invariant.end intrinsics currently
support specifying invariant memory objects only in the default address space.

With this change, these intrinsics are overloaded for any adddress space for memory objects
and we can use these llvm invariant intrinsics in non-default address spaces.

Example: llvm.invariant.start.p1i8(i64 4, i8 addrspace(1)* %ptr)

This overloaded intrinsic is needed for representing final or invariant memory in managed languages.

Diff Detail

Repository
rL LLVM

Event Timeline

anna updated this revision to Diff 64508.Jul 19 2016, 8:35 AM
anna retitled this revision from to Invariant start/end intrinsics overloaded for address space.
anna updated this object.
anna added a subscriber: llvm-commits.
anna updated this object.Jul 19 2016, 8:41 AM
anna added reviewers: reames, apilipenko.
anna set the repository for this revision to rL LLVM.

Ran regression tests on all supported platforms.

apilipenko edited edge metadata.Jul 21 2016, 7:35 AM

Please add a test case for autoupgrade functionality. See auto_upgrade_intrinsics.ll for examples.

docs/LangRef.rst
11845 ↗(On Diff #64508)

I'd omit this for the sake of consistency with other intrinsics.

lib/IR/AutoUpgrade.cpp
155 ↗(On Diff #64508)

I'd extract locals for Arg[1] and Arg[2]. The name of these variables will make it clear what the overloaded types are for.

anna updated this revision to Diff 64906.Jul 21 2016, 8:51 AM
anna marked an inline comment as done.
anna edited edge metadata.

Addressed comments above: added test for auto uppgrade, localized args in Auto upgrade code.

docs/LangRef.rst
11845 ↗(On Diff #64508)

I actually added the statement for the sake of consistency wth other overloaded intrinsics :)

All intrinsics that overload explicitly state that fact (examples: llvm.memcpy: This is an overloaded intrinsic. You can use llvm.memcpy on any
integer bit width and for different address spaces.)

apilipenko accepted this revision.Jul 21 2016, 8:57 AM
apilipenko edited edge metadata.

LGTM

lib/IR/AutoUpgrade.cpp
154 ↗(On Diff #64906)

It's a Type not a Value, right? That's not obvious. I'd name it accordingly or/and use the type instead of auto.

This revision is now accepted and ready to land.Jul 21 2016, 8:57 AM
anna updated this revision to Diff 64933.Jul 21 2016, 11:26 AM
anna edited edge metadata.

changed from auto to type *.

This revision was automatically updated to reflect the committed changes.
anna marked an inline comment as done.Jul 21 2016, 12:22 PM

Reverted change in r276320, due to failure in clang.
Assertion failure:
clang: /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/include/llvm/ADT/ArrayRef.h:196: const T &llvm::ArrayRef<llvm::Type *>::operator[](size_t) const [T = llvm::Type *]: Assertion `Index < Length && "Invalid index!"' failed.

anna reopened this revision.Jul 21 2016, 12:32 PM

The fix for build break is to pass in array of Type* to Intrinsic::getDeclaration and Intrinsic::getName.

This revision is now accepted and ready to land.Jul 21 2016, 12:32 PM
anna updated this revision to Diff 64944.Jul 21 2016, 1:01 PM

fix for clang build break: create array of Type* containing the single element,
and passing this Type*[] into Intrinsic::getDeclaration and Intrinsic::getName

anna closed this revision.Jul 22 2016, 11:52 AM

Committed llvm changes in https://reviews.llvm.org/rL276447

Commited clang changes in: https://reviews.llvm.org/rL276448

No clang failures.

anna added a comment.Jul 22 2016, 1:40 PM
In D22519#493183, @anna wrote:

Committed llvm changes in https://reviews.llvm.org/rL276447

Commited clang changes in: https://reviews.llvm.org/rL276448

No clang failures.

The clang build break was due to the absence of overloaded type in the function call for getting the invariant intrinsic in clang::CodeGen::CodeGenFunction::EmitDeclInvariant. Clang test failures are due to the insertion of llvm.invariant.start calls after the constructor. This is turned on only at O1.
Discussed with Artur offline and the fix being simple and similar to the llvm invariant intrinsic change in AutoUpgrade, is checked in to clang directly. All clang and llvm buildbots pass.