This is an archive of the discontinued LLVM Phabricator instance.

Split IntrReadArgMem into IntrReadMem and IntrArgMemOnly
ClosedPublic

Authored by nhaehnle on Apr 19 2016, 3:34 PM.

Details

Summary

IntrReadWriteArgMem simply becomes IntrArgMemOnly.

So there are fewer intrinsic properties that express their orthogonality
better, and correspond more closely to the corresponding IR attributes.

Suggested by: Philip Reames

Diff Detail

Repository
rL LLVM

Event Timeline

nhaehnle updated this revision to Diff 54282.Apr 19 2016, 3:34 PM
nhaehnle retitled this revision from to Split IntrReadArgMem into IntrReadMem and IntrArgMemOnly.
nhaehnle updated this object.
nhaehnle added reviewers: mehdi_amini, reames.
nhaehnle added a subscriber: llvm-commits.

Are autoupgrader patches needed?

mehdi_amini edited edge metadata.Apr 19 2016, 3:46 PM

Are autoupgrader patches needed?

Not totally sure but I thought we don't store intrinsic attributes in the bitcode and instead apply the one defined in table-gen any time you create a function for which the name matches the intrinsic?

This change should be totally unrelated to IR bitcode. The Intr* properties are only ever seen by TableGen. TableGen maps the Intr* properties to IR attributes in IntrinsicEmitter::EmitAttributes, but that mapping is unaffected by the change at hand. The attribute emission code could be cleaned up as well, but I'd rather do that separately, see also D18714.

reames accepted this revision.Apr 20 2016, 5:40 PM
reames edited edge metadata.

LGTM. I have tried to review all the changes, but it's possible I missed something in the mechanical parts. I'm assuming author did this via find/replace or other tooling so that chances of typos are low.

The naming and style comments would be best addressed in a separate commit.

include/llvm/IR/Intrinsics.td
34 ↗(On Diff #54282)

The (unless also...) part is confusing. Possibly, "it cannot be moved across aliasing potentially stores"?

utils/TableGen/CodeGenTarget.cpp
451 ↗(On Diff #54282)

For clarify, should this become ReadWriteAllMem?

577 ↗(On Diff #54282)

This appears to be correct, but slightly confusing. I don't see an obvious way to make it better other than possibly switching from using ModRefBehavior names to explicitly ORd bits. Eg.

Start with ModRef = RWA;
...
else if (Property->getName() == "IntrWriteMem")

ModRef = ModRefBehavior(ModRef & ~MR_Ref);
This revision is now accepted and ready to land.Apr 20 2016, 5:40 PM

Yes, the bulk of changes was done by a simple sed script after skimming a grep result to look for outliers.

I'm going to commit with your other suggestions except the ReadWriteAllMem (perhaps AnyMem?), because that seems a bit much in one change.

This revision was automatically updated to reflect the committed changes.