This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Let combineLoadToNewType preserve ABI alignment of the load (PR44543)
ClosedPublic

Authored by aqjune on Jan 14 2020, 8:11 AM.

Details

Summary

If aligment on LoadInst isn't specified, load is assumed to be ABI-aligned.
And said aligment may be different for different types.
So if we change load type, but don't pay extra attention to the aligment
(i.e. keep it unspecified), we may either overpromise (if the default aligment
of the new type is higher), or underpromise (if the default aligment
of the new type is smaller).

Thus, if no alignment is specified, we need to manually preserve the implied ABI alignment.

This addresses https://bugs.llvm.org/show_bug.cgi?id=44543 by making combineLoadToNewType preserve ABI alignment of the load.

Event Timeline

aqjune created this revision.Jan 14 2020, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 8:11 AM
aqjune updated this revision to Diff 237988.Jan 14 2020, 8:14 AM

Minor update

lebedev.ri retitled this revision from Let combineLoadToNewType preserve ABI alignment of the load to [InstCombine] Let combineLoadToNewType preserve ABI alignment of the load (PR44543).Jan 14 2020, 8:34 AM
lebedev.ri added inline comments.Jan 14 2020, 8:37 AM
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
465

Align

466
// If old load did not have an explicit alignment specified,
// manually preserve the implied (ABI) alignment of the load.
// Else we may inadvertently incorrectly over-promise alignment.
llvm/test/Transforms/InstCombine/load-bitcast64.ll
3

Please precommit the test changes first, and please use the script to autogenerate check lines

aqjune updated this revision to Diff 238003.Jan 14 2020, 9:19 AM

Address comments

aqjune marked an inline comment as done.Jan 14 2020, 9:21 AM
aqjune added inline comments.
llvm/test/Transforms/InstCombine/load-bitcast64.ll
3

Ok, I'll make a separate commit for this.

aqjune marked an inline comment as done.Jan 14 2020, 9:25 AM
aqjune added inline comments.
llvm/test/Transforms/InstCombine/load-bitcast64.ll
3

Or do you prefer having the auto generated check in this patch?

lebedev.ri added inline comments.Jan 14 2020, 9:29 AM
llvm/test/Transforms/InstCombine/load-bitcast64.ll
3

First, change the datalayout and autogenerate the check lines here, and commit that.
Then, rebase this patch to only show the changes caused by the fix itself.

aqjune marked an inline comment as done.Jan 14 2020, 10:03 AM
lebedev.ri edited the summary of this revision. (Show Details)Jan 14 2020, 10:06 AM

LG, thank you.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
466

Please do mark done comments as done. (the checkbox)

llvm/test/Transforms/InstCombine/load-bitcast64.ll
43–44

Nice

aqjune marked 3 inline comments as done.Jan 14 2020, 10:16 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 14 2020, 10:23 AM
This revision was automatically updated to reflect the committed changes.

Just noticed that I landed this without explicit accept; sorry.
PR44543 is now resolved.

Just noticed that I landed this without explicit accept; sorry.
PR44543 is now resolved.

(In this case i did accept it, it got lost on my side)