This is an archive of the discontinued LLVM Phabricator instance.

Emit argmemonly attribute for intrinsics
ClosedPublic

Authored by igor-laevsky on Jul 20 2015, 7:58 AM.

Details

Summary

This is clean up after adding argmemonly attribute.

This change teaches TableGen to emit argmemonly attribute when intrinsics are marked with ReadArgMem or ReadWriteArgMem flags. This will allow to remove GET_INTRINSIC_MODREF_BEHAVIOR in a follow up patches.

Diff Detail

Repository
rL LLVM

Event Timeline

igor-laevsky retitled this revision from to Emit argmemonly attribute for intrinsics.
igor-laevsky updated this object.
igor-laevsky added reviewers: hfinkel, reames.
igor-laevsky set the repository for this revision to rL LLVM.
igor-laevsky added a subscriber: llvm-commits.
reames edited edge metadata.Jul 21 2015, 3:11 PM

I can't point at a particular problem in the code, but the changes in the test files posted looks highly suspicious. I strongly suspect this patch is buggy. If you're convinced it's not, please explain the test changes I highlighted.

test/Analysis/BasicAA/cs-cs.ll
239 ↗(On Diff #30158)

The fact you're having to add argument sets containing anything other than the attributes which were there before and your argmemonly attribute is highly suspicious. I suspect this is the symptom of a bug.

test/Transforms/Inline/inline_invoke.ll
347 ↗(On Diff #30158)

Again, your change should be splitting existing attribute sets, not adding new combination of existing attributes.

utils/TableGen/CodeGenIntrinsics.h
63 ↗(On Diff #30158)

You can add the name without repeating the type if desired:
enum ModRefKind { .. } ModRef;

utils/TableGen/IntrinsicEmitter.cpp
708 ↗(On Diff #30158)

I would expect to see this table disappear. All of the information in it should now be driven by attributes right?

igor-laevsky added inline comments.Jul 22 2015, 7:45 AM
test/Analysis/BasicAA/cs-cs.ll
239 ↗(On Diff #30158)

In original test not all attributes were specified explicitly in the IR file. What actually happens is that before my change there was this set of attributes:

attributes #0 = { nounwind readonly }
attributes #1 = { nounwind }
attributes #2 = { noinline nounwind readonly }
attributes #3 = { nounwind ssp }

And after my change:

attributes #0 = { nounwind readonly argmemonly }
attributes #1 = { nounwind argmemonly }
attributes #2 = { noinline nounwind readonly }
attributes #3 = { nounwind ssp }
attributes #4 = { nounwind }
test/Transforms/Inline/inline_invoke.ll
347 ↗(On Diff #30158)

Don't see a problem here. {nounwind} is now split into {nounwind} and {nounwind,argmemonly}

utils/TableGen/CodeGenIntrinsics.h
63 ↗(On Diff #30158)

I like style with two definitions more. But I really don't have a strong preference on this.

utils/TableGen/IntrinsicEmitter.cpp
708 ↗(On Diff #30158)

Yes, that the plan. I will remove usages of this table in a next few changes. After them it will become fully redundant.

I'd like to have someone more familiar with this code take a look before submission, but with the explanation of the test changes, the code LGTM.

test/Analysis/BasicAA/cs-cs.ll
239 ↗(On Diff #30158)

Ah! Please submit a separate change to make the existing tests more complete. That test cleanup change doesn't need review. That will make the diff far more obvious.

Thanks for explaining this.

Update for the more recent tree

majnemer accepted this revision.Aug 12 2015, 11:52 AM
majnemer added a reviewer: majnemer.
majnemer added a subscriber: majnemer.

LGTM with nits addressed.

utils/TableGen/IntrinsicEmitter.cpp
681 ↗(On Diff #31804)

Please clang-format this.

This revision is now accepted and ready to land.Aug 12 2015, 11:52 AM
This revision was automatically updated to reflect the committed changes.