This is an archive of the discontinued LLVM Phabricator instance.

[IR] Switch everything to use memory attribute
ClosedPublic

Authored by nikic on Oct 12 2022, 7:48 AM.

Details

Summary

This switches everything to use the memory attribute proposed in https://discourse.llvm.org/t/rfc-unify-memory-effect-attributes/65579. The old argmemonly, inaccessiblememonly and inaccessiblemem_or_argmemonly attributes are dropped. The readnone, readonly and writeonly attributes are restricted to parameters only.

The old attributes are auto-upgraded both in bitcode and IR. The bitcode upgrade is a policy requirement that has to be retained indefinitely. The IR upgrade is mainly there so it's not necessary to update all tests using memory attributes in this patch, which is already large enough. We could drop that part after migrating tests, or retain it longer term, to make it easier to import IR from older LLVM versions.

High-level Function/CallBase APIs like doesNotAccessMemory() or setDoesNotAccessMemory() are mapped transparently to the memory attribute. Code that directly manipulates attributes (e.g. via AttributeList) on the other hand needs to switch to working with the memory attribute instead.

Depends on D135592.

Diff Detail

Event Timeline

nikic created this revision.Oct 12 2022, 7:48 AM
nikic requested review of this revision.Oct 12 2022, 7:48 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
nikic updated this revision to Diff 468924.Oct 19 2022, 8:23 AM

Rebase. This patch is still incomplete.

nikic updated this revision to Diff 469251.Oct 20 2022, 9:04 AM

Add bitcode autoupgrade support.

nikic updated this revision to Diff 469615.Oct 21 2022, 8:22 AM

The only missing part now is Attributor support.

Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

The only missing part now is Attributor support.

What is missing there?

nikic added a comment.Oct 21 2022, 8:28 AM

The only missing part now is Attributor support.

What is missing there?

The interaction of AAMemoryBehavior and AAMemoryLocation with the IR attributes. They need to initialized based on the memory attribute now and also manifest their results that way.

The only missing part now is Attributor support.

What is missing there?

The interaction of AAMemoryBehavior and AAMemoryLocation with the IR attributes. They need to initialized based on the memory attribute now and also manifest their results that way.

You want me to look into it? I didn't expect this to be much more complicated than the FunctionAttr pass. Maybe I'm wrong.

nikic added a comment.Oct 21 2022, 1:39 PM

The only missing part now is Attributor support.

What is missing there?

The interaction of AAMemoryBehavior and AAMemoryLocation with the IR attributes. They need to initialized based on the memory attribute now and also manifest their results that way.

You want me to look into it? I didn't expect this to be much more complicated than the FunctionAttr pass. Maybe I'm wrong.

FunctionAttrs is very simple because it already uses MemoryEffects as the internal representation, so it's mostly just a matter of deleting no longer needed translation code. If you have time to implement Attributor support that would be great, otherwise I'll look into this in more detail next week.

The only missing part now is Attributor support.

What is missing there?

The interaction of AAMemoryBehavior and AAMemoryLocation with the IR attributes. They need to initialized based on the memory attribute now and also manifest their results that way.

You want me to look into it? I didn't expect this to be much more complicated than the FunctionAttr pass. Maybe I'm wrong.

FunctionAttrs is very simple because it already uses MemoryEffects as the internal representation, so it's mostly just a matter of deleting no longer needed translation code. If you have time to implement Attributor support that would be great, otherwise I'll look into this in more detail next week.

I can't promise but I'll try. We can also check-in via discourse or so next week.

nikic updated this revision to Diff 470130.Oct 24 2022, 6:46 AM
nikic edited the summary of this revision. (Show Details)
nikic edited reviewers, added: nhaehnle; removed: ftynse, dcaballe.

Add attributor support. This is somewhat hacky, because both AAMemoryLocation and AAMemoryBehavior want to manifest the memory attribute, so we do that by intersecting with an existing one. I think doing this cleanly would require effectively merging AAMemoryBehavior into AAMemoryLocation, but for now this seems to work.

I think this should be ready for review now.

jdoerfert added inline comments.Oct 24 2022, 12:31 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
7370 ↗(On Diff #470130)

Can you leave a TODO or two here. We need to de-duplicate the code with the copy above and probably rethink what we emit, e.g., if we could emit better information.

7683 ↗(On Diff #470130)

This part (I think) broke a test, see below.

I'll assume the |= works as required here (though I find it counter-intuitive).
The dropping of the attributes feels complex nevertheless. Can't we just drop the memory attribute all together?
Can we verify the attribute is dropped from the IR?

7734 ↗(On Diff #470130)

Probably again a TODO.

llvm/test/Transforms/Attributor/memory_locations.ll
709 ↗(On Diff #470130)

This is a test to catch the instcombine + argmemonly problem and we broke it.
The TUNIT case is correct but the CGSCC case is not. Both line 685 and 701 are wrong.

nikic updated this revision to Diff 470415.Oct 25 2022, 2:04 AM
nikic marked 2 inline comments as done.

Address review comments

nikic added inline comments.Oct 25 2022, 2:05 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
7683 ↗(On Diff #470130)

Yeah, this code was incorrect. What I actually meant to do here is something like ME |= MemoryEffects(MemoryEffects::Other, ME.getModRef()), to indicate that this might potentially access a global as well.

In the interest of keeping this as close to NFC as possible, what I ended up doing is to preserve only the mod/ref information, and drop the location information, which should match what the code effectively did previously.

I read through all of the Transform changes and most of the IR changes. Some minor comments, Attributor stuff looks good, OpenMP changes too.

llvm/include/llvm/IR/InstrTypes.h
2334

IS this support moved somewhere? Do we support this for in-tree code?

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1722

Should we keep a comment to make sure the numbersare not reused?

llvm/lib/Transforms/Scalar/SCCP.cpp
604

This is a little more pessimistic than the original version but I doubt we should be too concerned. We should rather revisit this as we introduce the "globals"/"constants" category. Then we can also remain the read/write-only effect for them (or above for Other).

llvm/lib/Transforms/Utils/CodeExtractor.cpp
904 ↗(On Diff #470415)

Don't we have to add memory?

nikic updated this revision to Diff 470758.Oct 26 2022, 2:38 AM
nikic marked 3 inline comments as done.

Address review

llvm/include/llvm/IR/InstrTypes.h
2334

This is moved into getMemoryEffects(), which properly handles operand bundle effects. (And things like doesNotAcessMemory() use getMemoryEffects() internally, so they also handle this correctly.)

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1722

This attribute encoding is no longer used (only exists for auto-upgrade), but sure, a comment won't hurt...

llvm/lib/Transforms/Scalar/SCCP.cpp
604

I've tweaked this to copy the ModRef from ArgMem. Dropping the read/write info here wasn't intentional...

And yes, we can do better here after splitting "Other".

llvm/lib/Transforms/Utils/CodeExtractor.cpp
904 ↗(On Diff #470415)

It's already there, right before the continue. This was added in the previous patch.

Ordinarily, I would say this patch is a bit too big for comfort. That said, I did look at most of the llvm changes minus the attributor and sampled some of the test changes, and it looked good to me.

nikic added a comment.Oct 27 2022, 2:40 AM

Ordinarily, I would say this patch is a bit too big for comfort. That said, I did look at most of the llvm changes minus the attributor and sampled some of the test changes, and it looked good to me.

Something I considered originally was to try and support both the old attributes and the new one at the same time, to allow a more incremental change. However, it is hard to make sure that the resulting scheme is correct, e.g. code that drops things like argmemonly for correctness reasons would also have to drop it from the memory attribute, and doing that transparently would only be possible if we add a big backwards-compatibility layer into the core AttributeList and AttributeSet implementation, which maps things like "remove ArgMemOnly" to a more complex operation. Trying to do that seemed like a pretty bad idea. It also wouldn't do anything about the test churn, which makes up most of the patch. I don't have good ideas on how to split this down further.

Thank you for the additional explanation for not splitting up. This *is* likely to cause a little annoyance for downstreams, but your reasoning makes sense to me and so I'm not going to stand in the way :)

To be clear, you can count my earlier comment like an "Acked-by" and partial review. Combined with @jdoerfert's look earlier, I'd be satisfied for the LLVM parts. I can't speak for Clang, though.

nikic updated this revision to Diff 471944.Oct 31 2022, 3:07 AM

Drop old attributes from LangRef and add ReleaseNotes.

nikic added a subscriber: efriedma.

Adding @efriedma as a possible reviewer for the clang codegen part of these changes.

nikic updated this revision to Diff 472633.Nov 2 2022, 8:51 AM

Rebase over test changes.

jdoerfert accepted this revision.Nov 4 2022, 12:16 AM

I read through all the clang changes and clang test changes. Also again through all the LLVM changes (not through all the tests though).
I think this is fine. Minor nits below. I'll accept this now as there seems to be no objection.

clang/test/CodeGen/function-attributes.c
114

Where does the willreturn come from? *thinking* Oh, it just hid in the {{.*}} part. Never-mind.

llvm/docs/LangRef.rst
1423 ↗(On Diff #472633)

Newline

llvm/lib/IR/Instructions.cpp
383

C++17, cool.

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
1476

This seems more pessimistic. Unsure if it makes a difference.

This revision is now accepted and ready to land.Nov 4 2022, 12:16 AM
nikic marked 5 inline comments as done.Nov 4 2022, 1:56 AM
nikic added inline comments.
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
1476

It likely doesn't matter in practice, but I should at least add a TODO...

This revision was landed with ongoing or failed builds.Nov 4 2022, 2:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 2:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript