This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Unify AAMemoryLocation and AAMemoryBehavior
Needs ReviewPublic

Authored by jdoerfert on Jun 19 2023, 4:12 PM.

Details

Summary
NOTE: I still need to double check all test changes.

Since we added the memory attribute it was clear that AAMemoryLocation
and AAMemoryBehavior should be merged. We do this now and replace all
the custom bit-fiddling with the existing logic in llvm::MemoryEffects.
Since we want to track more locations, we made that class extensible in
https://reviews.llvm.org/D153305.

The new AA is a combination of the former two, though we tried to reduce
the compile time during the rewrite. We do not track accesses
transitively anymore (so a call site needs to be checked by the user of
checkForAllAccessesToMemoryKind). We also use a single
AAMemoryLocation for a call site or function to model the call
site/function as well as the arguments.

Diff Detail

Event Timeline

jdoerfert created this revision.Jun 19 2023, 4:12 PM
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert requested review of this revision.Jun 19 2023, 4:12 PM
Herald added a project: Restricted Project. · View Herald Transcript

Didn't make it all the way, will fix the issues below first.

llvm/test/Transforms/Attributor/ArgumentPromotion/X86/thiscall.ll
42 ↗(On Diff #532764)

TODO: Missing readnone here. Should not be impacted by the inalloca.

llvm/test/Transforms/Attributor/ArgumentPromotion/alignment.ll
25

Note: I put logic in to avoid argmem: mode if there are no pointer arguments but for now that logic won't trigger if we replaced/deleted pointer arguments.

llvm/test/Transforms/Attributor/ArgumentPromotion/array.ll
35

TODO: missing read-only here causes us to miss out on the privatization/promotion.

jdoerfert updated this revision to Diff 532782.Jun 19 2023, 9:32 PM

Fix firsts two TODOs: Read known information not only from Attribute::memory but also read/write/only/none.

Handle complex attributes better (inalloca, byval,...) and prop inaccessible when we give up on a call.

jdoerfert added inline comments.Jun 21 2023, 12:35 AM
llvm/test/Transforms/Attributor/value-simplify-pointer-info.ll
2173

Started at the bottom, made it till here, left comments.

llvm/test/Transforms/Attributor/value-simplify.ll
453–461

This should be better, preallocated writes (according to some existing test), IIRC.

489

I'm not sure if we need to go back to the write only. Probably worth to be conservative.

1266

This seems to be correct, as is the memory(none) annotation of memcpy_uses_store_caller

nikic added inline comments.Jun 21 2023, 1:51 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
4826

Will overflow with many args?

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
8716

Does this handle volatile loads correctly? Those can also write (and access inaccessible memory).

arsenm added inline comments.Jun 22 2023, 10:59 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
4826

If this is really a hard argument limit, it's way too small. llvm-reduce likes to move instructions to arguments, so I'm sure I'll run into something where this overflows in no time

jdoerfert added inline comments.Jun 24 2023, 4:05 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
4826

Fair, I make it bigger.

4826

I'll get rid of the indirection. AA::MemoryEffects is 32 bit, no real savings here even for "many" non-pointer args.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
8716

This was already wrong in the old code, code catch. That said, volatile should IMHO not access inaccessible memory. That seems like a contradiction to me since it would be an accessible then. I can make it access (=read/write) everything, but I think we should revisit that.

jdoerfert updated this revision to Diff 536092.Jun 29 2023, 8:14 PM

Rebase, fix minor errors. Remove the indirection (char vector).

jdoerfert added inline comments.Jun 29 2023, 8:29 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
1963

This was accidentally included. Will remove it. Ignore for now.

llvm/lib/Transforms/IPO/Attributor.cpp
1430

This was accidentally included. Will remove it. Ignore for now.

llvm/test/Transforms/Attributor/ArgumentPromotion/byval-2.ll
37

This is a curious case. I think, the result is sound in some sense, but not as the lang ref is Witten.

%X is read (%tmp2 in TUNIT below, or in the original call to make the byval copy), but the result is unused.
The original byval argument %X is only written by the callee, which makes it readnone on this side of the call, conceptually.

All that said, I need to check this in detail and avoid it. Lang ref should say violating read none results in poison, but it says it results in UB.

llvm/test/Transforms/Attributor/ArgumentPromotion/inalloca.ll
11

I think we are more conservative wrt. inalloca. Which is fine with me, given that I do not understand it.

llvm/test/Transforms/Attributor/ArgumentPromotion/variadic.ll
34

I am not sure if the byval should be a forced read. I guess that might solve the problem above as well.

jdoerfert updated this revision to Diff 536100.Jun 29 2023, 8:59 PM
jdoerfert marked 3 inline comments as done.

Force byval reads. Conservative handling for volatile store.

arsenm added inline comments.Aug 14 2023, 2:50 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
811–813

return ternary operator?