This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] fold LoadInst for uniformaly initialized constant global variables
ClosedPublic

Authored by khei4 on Feb 16 2023, 5:30 AM.

Diff Detail

Event Timeline

khei4 created this revision.Feb 16 2023, 5:30 AM
khei4 requested review of this revision.Feb 16 2023, 5:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2023, 5:30 AM
khei4 updated this revision to Diff 497984.Feb 16 2023, 5:32 AM

add tests

nikic added a comment.Feb 16 2023, 5:39 AM

Looks basically fine to me.

llvm/lib/Analysis/InstructionSimplify.cpp
6590

nit: regardless of?

6593

nit: Add a newline after this.

llvm/test/Transforms/InstSimplify/load.ll
53 ↗(On Diff #497984)

I would add an additional test that shows that this can look through multiple GEPs and doesn't depend on the GEP source element type ([4 x i32]) either.

%gep1 = getelementptr inbounds i8, ptr @constzeroarray, i64 %idx1
%gep = getelementptr inbouds i8, ptr %gep1, i64 %idx2
khei4 updated this revision to Diff 498137.Feb 16 2023, 1:56 PM

Fix comments and add test cases.

nikic added a comment.Feb 17 2023, 5:38 AM

I think something went wrong with the patch upload, it seems like only part of the test diff is there?

llvm/test/Transforms/InstSimplify/load.ll
64 ↗(On Diff #498137)

Using i64 0 doesn't really make sense, because it is a no-op. The instruction will be removed before we get to the fold. It should use a variable index as well.

It's also fine to reuse constzeroarray rather than constzeroarrayi8 here -- in fact, that shows that we don't need a matching stride between the getelementptr and the globals declaration. (We do need it for the non-zero initializer case.)

87 ↗(On Diff #498137)

Same here, getelementptr with zero index is a no-op.

khei4 updated this revision to Diff 498548.EditedFeb 17 2023, 6:34 PM
khei4 marked 2 inline comments as done.

I rebased commits and created a new diff. Maybe the cause is I combine the test commit and functional commit in the wrong way. I'm going to separate them next time.

nikic accepted this revision.Feb 18 2023, 5:41 AM

LGTM. Could you please share the Name <email> to use for the commit?

This revision is now accepted and ready to land.Feb 18 2023, 5:41 AM
khei4 added a comment.EditedFeb 18 2023, 5:08 PM

Thanks a lot for review! Please use Kohei Asano <kk.asano.luxy@gmail.com>.

llvm/test/Transforms/InstSimplify/load.ll
64 ↗(On Diff #498137)

Using i64 0 doesn't really make sense, because it is a no-op. The instruction will be removed before we get to the fold. It should use a variable index as well.

It's also fine to reuse constzeroarray rather than constzeroarrayi8 here -- in fact, that shows that we don't need a matching stride between the getelementptr and the globals declaration. (We do need it for the non-zero initializer case.)

Thanks. Used other variable indexes and reused constzeroarray for the second test case.

53 ↗(On Diff #497984)

Seems reasonable. Thanks for the review!

khei4 updated this revision to Diff 498708.Feb 19 2023, 6:20 PM

I noticed mistakenly reverted the review fix. Rebase.

This revision was landed with ongoing or failed builds.Feb 20 2023, 12:44 AM
This revision was automatically updated to reflect the committed changes.