This is an archive of the discontinued LLVM Phabricator instance.

tests: add attributes that would normally come from inferattrs
ClosedPublic

Authored by durin42 on Apr 4 2022, 3:17 PM.

Details

Summary

As my goal is to remove at least _some_ functions from the static list
in MemoryBuiltins.cpp, these tests either need to run inferattrs or
statically declare these attributes to keep passing. A couple of tests
had alternate cases which are no longer meaningful, e.g.
malloc-load-removal.ll.

Diff Detail

Unit TestsFailed

Event Timeline

durin42 created this revision.Apr 4 2022, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 3:17 PM
durin42 requested review of this revision.Apr 4 2022, 3:17 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
durin42 updated this revision to Diff 420856.Apr 6 2022, 7:41 AM
durin42 edited the summary of this revision. (Show Details)
durin42 updated this revision to Diff 420917.Apr 6 2022, 9:10 AM
durin42 updated this revision to Diff 421209.Apr 7 2022, 7:36 AM
durin42 updated this revision to Diff 422244.Apr 12 2022, 8:33 AM
durin42 updated this revision to Diff 424970.Apr 25 2022, 10:55 AM
durin42 updated this revision to Diff 425069.Apr 25 2022, 5:15 PM
durin42 updated this revision to Diff 425363.Apr 26 2022, 5:34 PM
durin42 updated this revision to Diff 425495.Apr 27 2022, 6:05 AM
durin42 updated this revision to Diff 425838.Apr 28 2022, 10:34 AM
durin42 updated this revision to Diff 426086.Apr 29 2022, 9:29 AM
durin42 updated this revision to Diff 426406.May 2 2022, 7:04 AM
durin42 updated this revision to Diff 426731.May 3 2022, 8:47 AM
durin42 updated this revision to Diff 426773.May 3 2022, 10:45 AM
durin42 updated this revision to Diff 427752.May 6 2022, 2:40 PM
nikic added a comment.May 18 2022, 6:57 AM

I think the general preference would be to explicitly add necessary attributes to test cases, rather than running inferattrs. In some of these cases it's not clear to me why the test would need extra attributes, e.g. in case of llvm/test/Transforms/MemCpyOpt/memcpy.ll -- it does test @malloc, but I don't think it needs anything beyond the noalias attribute that's already present.

durin42 updated this revision to Diff 431698.May 24 2022, 9:45 AM
durin42 retitled this revision from tests: enable inferattrs in many tests that will need it soon to tests: add attributes that would normally come from inferattrs.
durin42 edited the summary of this revision. (Show Details)

I think the general preference would be to explicitly add necessary attributes to test cases, rather than running inferattrs. In some of these cases it's not clear to me why the test would need extra attributes, e.g. in case of llvm/test/Transforms/MemCpyOpt/memcpy.ll -- it does test @malloc, but I don't think it needs anything beyond the noalias attribute that's already present.

Done. A couple of variants of some tests became nonsense (as mentioned in the log message) but for the most part this was straightforward. PTAL?

(This also now comes later in the series, since it has to happen after allockind() exists.)

@nikic where do I stand on this series? Am I waiting on you or should I try and rustle up another reviewer?

nikic added a comment.Jun 7 2022, 1:16 PM

This looks good, just wondering whether we can reduce to only allocsize for some of these?

llvm/test/Instrumentation/BoundsChecking/simple.ll
13

Do we need more than allocsize for this test?

llvm/test/Transforms/InstCombine/objsize-64.ll
5

Does this and the following test need more than allocsize?

llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-phi.ll
8

Does this need more than just allocsize(0)?

llvm/test/Transforms/MemCpyOpt/memcpy.ll
331

Still don't understand why this one would need extra attributes.

durin42 marked 4 inline comments as done.Jul 12 2022, 3:47 PM
durin42 added inline comments.
llvm/test/Transforms/InstCombine/objsize-64.ll
5

They seem to! If I golf them the tests eventually end up failing later in the series.

llvm/test/Transforms/MemCpyOpt/memcpy.ll
331

I think so that the analysis knows it's an uninitialized block. In any case, if I remove the allockind() the test fails once we're relying on attributes instead of the hard-coded list.

durin42 updated this revision to Diff 444094.Jul 12 2022, 3:47 PM
durin42 marked 2 inline comments as done.
durin42 updated this revision to Diff 446194.Jul 20 2022, 9:57 AM
durin42 updated this revision to Diff 446207.Jul 20 2022, 10:35 AM
durin42 updated this revision to Diff 446563.Jul 21 2022, 10:24 AM
nikic accepted this revision.Jul 22 2022, 12:55 AM

LGTM

llvm/test/Transforms/MemCpyOpt/memcpy.ll
331

Oh duh, we need allockind to allow eliminating the dead malloc call. The allocsize is not necessary.

This revision is now accepted and ready to land.Jul 22 2022, 12:55 AM
nikic added inline comments.Jul 22 2022, 1:05 AM
llvm/test/Transforms/Attributor/heap_to_stack.ll
188 ↗(On Diff #446563)

This is missing an allocalign attribute. I think this will avoid the regression of this test in D130107.

durin42 updated this revision to Diff 447397.Jul 25 2022, 10:20 AM
durin42 marked an inline comment as done.Jul 25 2022, 10:20 AM

Nice catch on the missing allocalign!

This revision was landed with ongoing or failed builds.Jul 25 2022, 2:29 PM
This revision was automatically updated to reflect the committed changes.