This is an archive of the discontinued LLVM Phabricator instance.

Add IntrOnlyWrite intrinsic property
ClosedPublic

Authored by nhaehnle on Mar 18 2016, 8:04 PM.

Details

Summary

This property is used to mark an intrinsic that writes to memory, but is
lowered to instructions without the mayLoad or hasSideEffects flag because the
target-specific code understands its memory behavior fully.

An example for this is the llvm.amdgcn.buffer.store.format.* intrinsic, which
corresponds to a store instruction that goes through a special buffer
descriptor rather than through a plain pointer.

With this property, the intrinsic should still be handled as having side
effects at the LLVM IR level, but machine scheduling can make smarter
decisions.

Diff Detail

Event Timeline

nhaehnle updated this revision to Diff 51100.Mar 18 2016, 8:04 PM
nhaehnle retitled this revision from to Add IntrOnlyWrite intrinsic property.
nhaehnle updated this object.
nhaehnle added reviewers: tstellarAMD, arsenm.
nhaehnle added a subscriber: llvm-commits.
nhaehnle updated this revision to Diff 52424.Apr 1 2016, 2:19 PM

Reorganize the code a bit, taking into account some feedback via llvm-dev
and elsewhere.

This patch only adds the LLVM intrinsic property to keep it simple and limited
to the AMDGPU use case.

mehdi_amini edited edge metadata.Apr 1 2016, 2:51 PM

Looks OK in principle, but please see inline comments.

utils/TableGen/CodeGenIntrinsics.h
65

I found it confusing that these are used to define the enum value below and also separately in the code. Maybe use a separate enum?
Also documenting the lattice would be nice.
The individual value meaning might be worth a comment, for instance MR_Read is *read through argument only* from what I understand below (and the name does not convey this).

utils/TableGen/IntrinsicEmitter.cpp
603

Don't you get a warning for fully covered switch?

nhaehnle updated this revision to Diff 52436.Apr 1 2016, 3:38 PM
nhaehnle edited edge metadata.

Changes to the ModRefBehavior enum in TableGen.

nhaehnle marked an inline comment as done.Apr 1 2016, 3:38 PM
nhaehnle added inline comments.
utils/TableGen/CodeGenIntrinsics.h
65

The intention is to be analogous to the FunctionModRefBehavior in AliasAnalysis. The values of the bits are different to preserve the ordering of the possible numerical values.

I'm changing MR_Read/Write to MR_Ref/Mod to make this clearer, and putting the bits into their own enum.

utils/TableGen/IntrinsicEmitter.cpp
603

Not originally, because of the non-sensical possibility of MR_Anywhere without MR_Read/Ref or MR_Write/Mod. With the new enum organization it can indeed be removed. (I didn't get the warning even then, perhaps that's a gcc vs. clang thing.)

nhaehnle marked an inline comment as done.Apr 1 2016, 3:39 PM

No other comments on my side, let's wait for Philip's review.

reames accepted this revision.Apr 18 2016, 5:17 PM
reames edited edge metadata.

LGTM w/comments addressed.

include/llvm/IR/Intrinsics.td
46

A better approach would be to split the existing ReadArgMem into an ArgMemOnly attribute and the existing ReadMemOnly attribute. I will not require that for this change, but it would be a worthwhile cleanup.

48

The writes can not be volatile. We model a volatile write as both reading and writing from the address. Changing that would be a much larger change.

This revision is now accepted and ready to land.Apr 18 2016, 5:17 PM
This revision was automatically updated to reflect the committed changes.