This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Use the zero initializer instead of storing an all zero representation.
ClosedPublic

Authored by mattd on Jan 25 2018, 10:49 AM.

Details

Summary

This change avoids the overhead of storing, and later crawling,
an initializer list of all zeros for arrays. When LLVM
visits this (llvm/IR/Constants.cpp) ConstantArray::getImpl()
it will scan the list looking for an array of all zero.

We can avoid the store, and short-cut the scan, by detecting
all zeros when clang builds-up the initialization representation.

This was brought to my attention when investigating PR36030

Diff Detail

Repository
rC Clang

Event Timeline

mattd created this revision.Jan 25 2018, 10:49 AM

I'm somewhat surprised LLVM doesn't already canonicalize this, but ok.

Should we also do this when building a constant struct?

lib/CodeGen/CGExprConstant.cpp
873

Please short-circuit this if AllNullValues is already false.

mattd updated this revision to Diff 133497.Feb 8 2018, 2:54 PM

Thanks for the look @rjmccall . I tossed in the shortcut as you suggested. I also had to fix the instruction numbering in the test case.

mattd marked an inline comment as done.Feb 8 2018, 2:54 PM

Oh. It's not a good idea to try to match exact local IR names like this for two reasons:

  • First, many IR names are different in builds with and without assertions.
  • Second, it's pretty susceptible to innocuous changes in output.

You should use FileCheck patterns ([[TEMP:%.*]] = bitcast [10 x i32]* ... to define the variable, [[TEMP]] to refer to it) instead.

Also, you really have 15 different tests here. Please split them up into different functions (in the same test file) with at least vaguely meaningful names (like testStaticNonZero or testVolatileAllZeros). And please match their definition lines with CHECK-LABEL instead of a simple CHECK; this allows FileCheck to do a better job reporting multiple failures in a single file.

Finally, I'm not sure why you've included the b, c, and d tests at all, since they have nothing to do with your patch. c4 and d4 don't even involve initialization code.

mattd updated this revision to Diff 133540.Feb 8 2018, 5:10 PM

Thanks for the test tips. I realize the original was a bit carried away, my apologies for that. This updated test visits the same code path that we're trying to test, and is much more concise.

rjmccall accepted this revision.Feb 8 2018, 6:32 PM

No worries. LGTM!

This revision is now accepted and ready to land.Feb 8 2018, 6:32 PM
This revision was automatically updated to reflect the committed changes.