This is an archive of the discontinued LLVM Phabricator instance.

Add mrrc/mrrc2 intrinsics and update existing mcrr/mcrr2 intrinsics to accept a single uint64 type instead of 2 uint32 types
ClosedPublic

Authored by rs on Jun 9 2016, 5:07 AM.

Details

Summary

Patch adds support for the intrinsics mrrc/mrrc2 and updates the existing mcrr/mcrr2 intrinsic definition to accept a single uint64 type as the input value instead of 2 uint32's as I think this makes the definition more compact.

Diff Detail

Event Timeline

rs updated this revision to Diff 60161.Jun 9 2016, 5:07 AM
rs retitled this revision from to Add mrrc/mrrc2 intrinsics and update existing mcrr/mcrr2 intrinsics to accept a single uint64 type instead of 2 uint32 types.
rs updated this object.
rs added reviewers: t.p.northover, rengolin.
rs added a subscriber: cfe-commits.

LLVM part of the patch is here http://reviews.llvm.org/D21178

rengolin added inline comments.Jun 10 2016, 10:09 AM
lib/CodeGen/CGBuiltin.cpp
3817

Would be better to use the comments as names and avoid the comments...

It'd also make clear the casts below.

3824

These variables could have better names...

3846

Same here

test/CodeGen/builtins-arm.c
203–206

I'm assuming this is a "soon to be documented" move, right?

It's really hard to review patches with these things changing all the time...

rs marked 3 inline comments as done.Jun 13 2016, 4:34 AM
rs added inline comments.
test/CodeGen/builtins-arm.c
203–206

I'm assuming this is a "soon to be documented" move, right?

Yes I've written the document myself which defines these intrinsics.

It's really hard to review patches with these things changing all the time...

Sorry, I didn't know these sorts of thing change that often.

rs updated this revision to Diff 60504.Jun 13 2016, 4:36 AM

Hi Renato,

please look again at this patch, I've addressed your comments.

Thanks,
Ranjeet

rengolin accepted this revision.Jun 15 2016, 3:54 AM
rengolin edited edge metadata.

Thanks Ranjeet, looks much better now! LGTM, Thanks!

This revision is now accepted and ready to land.Jun 15 2016, 3:54 AM
This revision was automatically updated to reflect the committed changes.