This is an archive of the discontinued LLVM Phabricator instance.

[InferAlignment] Create tests for InferAlignment pass
ClosedPublic

Authored by 0xdc03 on Aug 22 2023, 10:51 AM.

Details

Summary

These tests are in preparation for the InferAlignment pass. They consist
mainly of tests that break when alignment inference is disabled in
LoadInst and StoreInst within InstCombine.

Diff Detail

Unit TestsFailed

Event Timeline

0xdc03 created this revision.Aug 22 2023, 10:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 10:51 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
0xdc03 requested review of this revision.Aug 22 2023, 10:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 10:51 AM
nikic added inline comments.Aug 22 2023, 12:22 PM
llvm/test/Transforms/InferAlignment/dbginfo-1.ll
2 ↗(On Diff #552433)

These dbginfo tests don't look relevant -- this pass doesn't do anything that affects debuginfo.

0xdc03 updated this revision to Diff 552635.Aug 23 2023, 2:21 AM
  • Remove unnecessary debuginfo tests
0xdc03 updated this revision to Diff 552668.Aug 23 2023, 5:23 AM
  • Reorder the patches
0xdc03 edited the summary of this revision. (Show Details)Aug 23 2023, 5:26 AM
nikic added a comment.Aug 24 2023, 9:02 AM

High level feedback on these tests: Due to where they came from, they are kind of all over the place. They are split across a large number of files, the test names are, to the most part, not meaningful for this pass, and occasionally there are comments that don't make sense in the context of this pass. I think this needs to be a lot more consolidated, with a better understanding of what each test is actually supposed to test.

llvm/test/Transforms/InferAlignment/addrspace.ll
10 ↗(On Diff #552668)

Test name doesn't make sense, there is no bitcast here (and no gep below).

llvm/test/Transforms/InferAlignment/basic.ll
57 ↗(On Diff #552668)

Comment makes no sense in the context of this pass.

0xdc03 updated this revision to Diff 553724.Aug 26 2023, 6:16 AM
0xdc03 edited the summary of this revision. (Show Details)
  • Make tests better
nikic added inline comments.Aug 27 2023, 10:02 AM
llvm/test/Transforms/InferAlignment/alloca.ll
22

Might make sense to add explicit , align 1 on the allocas, loads and stores here, to show that the i32s would also get increased.

36

Drop nounwind.

53

These are no-op GEPs. You probably want to produce a non-zero offset?

llvm/test/Transforms/InferAlignment/irregular-size.ll
5

Argument unused.

I'd also call this @non_pow2_size or so, though I'm not really clear on what this is support to test wrt alignment.

27

Should use non-zero GEP here.

llvm/test/Transforms/InferAlignment/param-attr.ll
3 ↗(On Diff #553724)

Drop triple (from all tests that have one).

25 ↗(On Diff #553724)

You can also add a test where align 32 is on the return value.

27 ↗(On Diff #553724)

Drop nounwind.

llvm/test/Transforms/InferAlignment/propagate-assume.ll
48

Can also add a test using an assume operand bundle (call void @llvm.assume(i1 true) ["aligned"(...)]).

llvm/test/Transforms/InferAlignment/vector.ll
3

I think gep-vector.ll and vector.ll should be combined into one file.

llvm/test/Transforms/InferAlignment/volatile.ll
3

Use default CHECK prefix.

llvm/test/Transforms/InferAlignment/vscale.ll
14

Use non-zero GEP.

0xdc03 updated this revision to Diff 554221.Aug 29 2023, 2:31 AM
  • Make tests betterer
  • Rebase on main
0xdc03 added inline comments.Aug 29 2023, 2:46 AM
llvm/test/Transforms/InferAlignment/irregular-size.ll
5

Argument unused.

I'd also call this @non_pow2_size or so, though I'm not really clear on what this is support to test wrt alignment.

I left this in because I felt it may be good to have testing for weird sizes so that nothing strange happens if something expects sizes to be power of two.

nikic added inline comments.Aug 29 2023, 2:52 AM
llvm/test/Transforms/InferAlignment/alloca.ll
48

This is going to index outside the alloca. What you want here is something like getelementptr %struct.pair, ptr %alloca.struct, i64 0, i32 1 which would skip over the first { i32, i32 }, i.e. apply an 8 byte offset.

0xdc03 updated this revision to Diff 554280.Aug 29 2023, 5:52 AM
  • Make tests betterer-er
    • Fix alignment for struct types not being inferred correctly in alloca.ll
    • Fix load_vector_i4 in irregular-size.ll and add more cases
    • Combine tests for load and store in vector.ll to reduce noise
nikic accepted this revision.Aug 29 2023, 7:51 AM

LGTM with two nits.

llvm/test/Transforms/InferAlignment/alloca.ll
48

This one is still going to index out of bounds (as we advance by two pairs total). I would change the index here to i64 0, i32 1 as well (and the expected alignment would then be align 4).

llvm/test/Transforms/InferAlignment/vscale.ll
13

These accesses are going to go out of bounds again. Maybe replace the allocas in this test with a ptr align N parameter just to make things kosher?

This revision is now accepted and ready to land.Aug 29 2023, 7:51 AM
0xdc03 updated this revision to Diff 554363.Aug 29 2023, 8:25 AM
  • Address reviewer comments
    • Fix out of bounds GEP
    • Also add a 8-byte aligned load/store to alloca_struct
0xdc03 marked 10 inline comments as done.Aug 29 2023, 8:27 AM
0xdc03 updated this revision to Diff 556956.Sep 18 2023, 9:48 AM
  • Rebase on main
This revision was automatically updated to reflect the committed changes.