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.
Details
Details
- Reviewers
nikic - Commits
- rG3978f37c0f1e: [InferAlignment] Create tests for InferAlignment pass
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| 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. |
Comment Actions
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. |
| llvm/test/Transforms/InferAlignment/alloca.ll | ||
|---|---|---|
| 21 | Might make sense to add explicit , align 1 on the allocas, loads and stores here, to show that the i32s would also get increased. | |
| 35 | Drop nounwind. | |
| 52 | These are no-op GEPs. You probably want to produce a non-zero offset? | |
| llvm/test/Transforms/InferAlignment/irregular-size.ll | ||
| 4 | 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. | |
| 26 | Should use non-zero GEP here. | |
| llvm/test/Transforms/InferAlignment/param-attr.ll | ||
| 3 | Drop triple (from all tests that have one). | |
| 25 | You can also add a test where align 32 is on the return value. | |
| 27 | Drop nounwind. | |
| llvm/test/Transforms/InferAlignment/propagate-assume.ll | ||
| 47 | Can also add a test using an assume operand bundle (call void @llvm.assume(i1 true) ["aligned"(...)]). | |
| llvm/test/Transforms/InferAlignment/vector.ll | ||
| 2 | I think gep-vector.ll and vector.ll should be combined into one file. | |
| llvm/test/Transforms/InferAlignment/volatile.ll | ||
| 2 | Use default CHECK prefix. | |
| llvm/test/Transforms/InferAlignment/vscale.ll | ||
| 13 | Use non-zero GEP. | |
| llvm/test/Transforms/InferAlignment/irregular-size.ll | ||
|---|---|---|
| 4 |
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. | |
| 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. | |
Comment Actions
- 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
Comment Actions
LGTM with two nits.
| llvm/test/Transforms/InferAlignment/alloca.ll | ||
|---|---|---|
| 49 | 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 | ||
| 14 | 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? | |
Comment Actions
- Address reviewer comments
- Fix out of bounds GEP
- Also add a 8-byte aligned load/store to alloca_struct
Might make sense to add explicit , align 1 on the allocas, loads and stores here, to show that the i32s would also get increased.