This is an archive of the discontinued LLVM Phabricator instance.

[FuncSpec] Fix inconsistent treatment of global variables
ClosedPublic

Authored by chill on Apr 28 2023, 12:28 PM.

Details

Summary

There are a few inaccuracies with how FuncSpec handles global variables.

When specialisation on non-const global variables is disabled (the default)
the pass could nevertheless perform some specializations, e.g. on a constant
GEP expression, or on a SSA variable, for which the Solver has determined
it has the value of a global variable.

When specialisation on non-const global variables is enabled, the pass would
skip non-scalars, e.g. a global array, but this should be completely inconsequential,
a pointer is a pointer.

Diff Detail

Event Timeline

chill created this revision.Apr 28 2023, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 12:28 PM
chill requested review of this revision.Apr 28 2023, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2023, 12:28 PM
SjoerdMeijer accepted this revision.May 2 2023, 12:42 AM

LGTM

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
744

Nit: does C not always have a value?

llvm/test/Transforms/FunctionSpecialization/compiler-crash-promote-alloca.ll
7

Nit: do we need this change? It might no longer test "compiler crash promote alloca"?

llvm/test/Transforms/FunctionSpecialization/global-var-constants.ll
50

Nit: this sentence does not really flow, perhaps some punctuation missing.

77

Nit: just check the numeric value here and also above on line 74?

This revision is now accepted and ready to land.May 2 2023, 12:42 AM
chill updated this revision to Diff 518697.May 2 2023, 5:58 AM
chill marked 2 inline comments as done.
chill marked 2 inline comments as done.May 2 2023, 6:03 AM
chill added inline comments.
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
744

It's fairly typical for C to be nullptr here, for example we call this functions with a parameter value, which is not a constant by itself, nor it is derived by the solver to be constant.

llvm/test/Transforms/FunctionSpecialization/compiler-crash-promote-alloca.ll
7

It still tests it, I reverted the fix and verified the compiler still crashes.

llvm/test/Transforms/FunctionSpecialization/global-var-constants.ll
50

All right, it might be a clumsy way to say the compiler does what it is requested to do.

What the compiler did with regard to non-const globals was that sometimes
such specialisations were:

  • not allowed -> performed
  • allowed -> not performed (even though other conditions were present)

The two comments, basically say:

  • not allowed -> not performed
  • allowed -> performed

It needs a comma before "then", I suppose.

This revision was landed with ongoing or failed builds.May 5 2023, 1:56 AM
This revision was automatically updated to reflect the committed changes.
chill marked an inline comment as done.