[PATCH] D114766: If constrained intrinsic is replaced, remove its side effect

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

In D114766#3160877 <https://reviews.llvm.org/D114766#3160877>, @nikic wrote:

> This is likely not going to be particularly useful, as the InstSimplifyPass is not really used outside of testing. To be generally applicable, this logic would have to be in wouldInstructionBeTriviallyDead().

Why? This pass is run when clang is executed. Due to it the source file:

  double func() {
    return 1.0 + 2.0;
  }

compiled with:

  clang -target x86_64-linux -S -O2 -ftrapping-math const-10.c

produces code:

  	movsd	.LCPI0_0(%rip), %xmm0           # xmm0 = mem[0],zero
  	retq

If setting attribute `ReadNone` in `mayFoldConstrained` is commented out, the resulting code contains add instruction:

  	movsd	.LCPI0_0(%rip), %xmm0           # xmm0 = mem[0],zero
  	addsd	.LCPI0_1(%rip), %xmm0
  	movsd	.LCPI0_2(%rip), %xmm0           # xmm0 = mem[0],zero
  	retq

But, as you noted in D110322 <https://reviews.llvm.org/D110322>, ConstantFolding.cpp is not right place for changing attributes. This patch just moves this change to more appropriate place.

Moving this logic to `wouldInstructionBeTriviallyDead()` does not seem right, because the decision about removing side effect comes from constant evaluation and not from the state of call object.

In D114766#3161426 <https://reviews.llvm.org/D114766#3161426>, @nikic wrote:

> Marking as changes requested per above comment -- really don't think this is the right place to address this, since it will just create an incorrect picture of what LLVM can optimize in practice. Usually, this fold will happen via InstCombine rather than InstSimplifyPass. But I don't think changing InstCombine is the right solution either, as any number of passes do this kind of "simplify and DCE" pattern.

Sorry, I don't understand why this is not the right place. This pass makes constant folding and it is natural to fix attributes here, isn't it? What is wrong?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114766/new/

https://reviews.llvm.org/D114766