Incorrect optimization of assignment in present of memset

[llvmbot]

Bugzilla Link	34088
Resolution	FIXED
Resolved on	Aug 10, 2017 18:54
Version	5.0
OS	Linux
Blocks	#33196
Attachments	An example to reproduce the problem.
Reporter	LLVM Bugzilla Contributor
CC	@topperc,@zmodem,@RKSimon

Extended Description

During optimization clang removes assignments that should not be eliminated.
I have the following code:

Init(&foo);
buffer = foo.bar.buffer;
memset(&foo, 0xCD, sizeof (foo));
foo.bar.buffer = buffer;
Print(&foo);

and if optimization is enabled, it becomes like this:

Init(&foo);
memset(&foo, 0xCD, sizeof (foo));
Print(&foo);

The bug happens only with target: i386-pc-linux-gnu and for some layout of
structures. (Perhaps, it can happen on x86_64 too for different structures,
but I have not encountered it only on i386 architecture.)

I have attached a full example that demonstrates the problem. It includes
the following files:
test1.h - defines some structures.
test1.c - the main code that is incorrectly optimized
test2.c - defines Init() and Print()
Makefile - builds and runs the example code

If optimization is not enabled, the program prints as expected:
ptr=(nil) size=0x0
however when any level optimization is enabled, I get the following output:
ptr=0xcdcdcdcd size=0xcdcdcdcd
i.e. the initial value of 'buffer' set in Init() is overwritten by memset.

clang information:
$ clang-5.0 -m32 --version
clang version 5.0.0-svn309965-1~exp1 (branches/release_50)
Target: i386-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

[topperc]

The problem seems to be that when we reach this code in combineStore in X86ISelLowering.cpp

// If we are a 64-bit capable x86, lower to a single movq load/store pair.
// Otherwise, if it's legal to use f64 SSE instructions, use f64 load/store
// pair instead.
if (Subtarget.is64Bit() || F64IsLegal) {
  MVT LdVT = Subtarget.is64Bit() ? MVT::i64 : MVT::f64;
  SDValue NewLd = DAG.getLoad(LdVT, LdDL, Ld->getChain(), Ld->getBasePtr(),
                              Ld->getPointerInfo(), Ld->getAlignment(),
                              Ld->getMemOperand()->getFlags());
  SDValue NewChain = NewLd.getValue(1);
  if (TokenFactorIndex >= 0) {
    Ops.push_back(NewChain);
    NewChain = DAG.getNode(ISD::TokenFactor, LdDL, MVT::Other, Ops);
  }
  return DAG.getStore(NewChain, StDL, NewLd, St->getBasePtr(),
                      St->getPointerInfo(), St->getAlignment(),
                      St->getMemOperand()->getFlags());
}

We create a new load, but don't make that new load's chain used by any other locations that used the old load we're replacing. Except for the use by the store.

I think this causes us to later believe that the store for the memset does not occur after the load.

Nirav, is this something you can look at? I suspect you know far more about the chains between loads and stores than I do.

[zmodem]

This sounds scary. Can someone confirm whether this is a regression or something that was broken for a long time?

[llvmbot]

Craig's insight is dead on; we are dropping dependencies in duplication. This appears to have been latent for years.

Fix here: https://reviews.llvm.org/D36528

[llvmbot]

Fixed @ rL310604

[zmodem]

Fixed @ rL310604

Thanks! I'll merge once it's been in the tree for a bit.

[zmodem]

Fixed @ rL310604

Thanks! I'll merge once it's been in the tree for a bit.

r310678