This is an archive of the discontinued LLVM Phabricator instance.

[Evaluator] Improve evaluation of load/store
ClosedPublic

Authored by evgeny777 on Feb 19 2018, 5:14 AM.

Details

Summary

When storing pointers in struct/class constructor clang often generates IR like this:

define internal void @_GLOBAL__sub_I_main.cpp()  {
  %1 = load i64, i64* bitcast (%struct.A** @gA to i64*), align 8
  store i64 %1, i64* bitcast (%struct.S* @s to i64*), align 8
  ret void
}

which corresponds to following C++ code:

extern A *gA;
struct S {
  A* _a;
} SObj(gA);

With D43077 it is now possible to import variables from different TU when using ThinLTO,
so it would be nice to optimize such static ctors out.

Diff Detail

Event Timeline

evgeny777 created this revision.Feb 19 2018, 5:14 AM
evgeny777 retitled this revision from [Evaluator] Improve evaluation of bitcast to [Evaluator] Improve evaluation of load/store.Feb 19 2018, 5:15 AM

Any comments on this?

Sorry for the delay. I don't feel super confident reviewing these change. I looked back at who touched the ComputeLoadResult code last (besides pcc who simply extracted it out of GlobalOpt into Utils), and it was Chris Lattner in 2005! So I am not completely sure who to add to as a reviewer at this point.
Poking around LLVM though, I see that there is some very similar handling in llvm::ConstantFoldLoadFromConstPtr (ConstantFolding.cpp). In particular, the handling of P as a GlobalVariable or GEP appears identical to what is already in Evaluator::ComputeLoadResult. It already has handling for bitcasts, although that handling appears quite a bit more complex that what you added to ComputeLoadResult. I'm wondering whether ComputeLoadResult can be refactored to just call ConstantFoldLoadFromConstPtr, or whether there is a reason it needs to remain distinct (and why the bitcast handling added here is simpler).

evgeny777 updated this revision to Diff 137443.Mar 7 2018, 11:30 AM

Unfortunately ConstantFoldLoadFromConstPtr seems to be useful, because it checks for GV initializer being constant and we're not forced by this constraint when evaluating static ctors.
However it turned out we can use ConstantFoldLoadThroughBitcast when evaluating both load and store.

tejohnson accepted this revision.Mar 8 2018, 6:37 AM

LGTM with some minor comments below.

include/llvm/Analysis/ConstantFolding.h
150 ↗(On Diff #137443)

document

lib/Transforms/Utils/Evaluator.cpp
202

The two opcode cases are both doing the same checks (is it a global variable with a definitive initializer), but the checking is structured differently. Suggest structuring the checks the same way for both opcodes.

This revision is now accepted and ready to land.Mar 8 2018, 6:37 AM
This revision was automatically updated to reflect the committed changes.