Fix PR40644: miscompile indexed FP constant store

[thopre]

Summary

Functions replaceStoreOfFPConstant() and OptimizeFloatStore() both
replace store of float by a store of an integer unconditionally. However
this generates wrong code when the store that is replaced is an indexed
or truncating store. This commit solves this issue by adding an early
return in these functions when the store being considered is not a
normal store.

Bug was only observed on out of tree targets, hence the lack of testcase
in this commit.

[JonChesterfield]

Yep, that's what I had in mind. Do you have regression tests in the GC backend for these?

[thopre]

In D68420#1693634, @JonChesterfield wrote:

Yep, that's what I had in mind. Do you have regression tests in the GC backend for these?

If I read correctly the commit that added the same logic in the backend, yes.

[lebedev.ri]

Tests missing.

[thopre]

In D68420#1693679, @lebedev.ri wrote:

Tests missing.

We've met this bug with an out of tree target. It requires a target with truncating or indexed constant float store which Eli Friedman was of the opinion is not the case of any of the in tree target. Still the change is small and obvious enough to be accepted. Both of these functions proceed with doing a regular integer store regardless of the initial store.

[thopre]

In D68420#1694954, @thopre wrote:

In D68420#1693679, @lebedev.ri wrote:

Tests missing.

We've met this bug with an out of tree target. It requires a target with truncating or indexed constant float store which Eli Friedman was of the opinion is not the case of any of the in tree target. Still the change is small and obvious enough to be accepted. Both of these functions proceed with doing a regular integer store regardless of the initial store.

Ping?

[efriedma]

Please add a note to the commit message about why there is no test.

LGTM

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::replaceStoreChain(StoreSDNode *ST, SDValue BetterChain) {
   return CombineTo(ST, Token, false);
 }
 
 SDValue DAGCombiner::replaceStoreOfFPConstant(StoreSDNode *ST) {
   SDValue Value = ST->getValue();
   if (Value.getOpcode() == ISD::TargetConstantFP)
     return SDValue();
 
+  if (!ISD::isNormalStore(ST))
+    return SDValue();
+
   SDLoc DL(ST);
 
   SDValue Chain = ST->getChain();
   SDValue Ptr = ST->getBasePtr();
 
   const ConstantFPSDNode *CFP = cast<ConstantFPSDNode>(Value);
 
   // NOTE: If the original store is volatile, this transform must not increase

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

if (Val.getValueType() == EltVT ||
 
       return DAG.getVectorShuffle(Vec.getValueType(), dl, Vec, ScVec, ShufOps);
     }
   }
   return PerformInsertVectorEltInMemory(Vec, Val, Idx, dl);
 }
 
 SDValue SelectionDAGLegalize::OptimizeFloatStore(StoreSDNode* ST) {
+  if (!ISD::isNormalStore(ST))
+    return SDValue();
+
   LLVM_DEBUG(dbgs() << "Optimizing float store operations\n");
   // Turn 'store float 1.0, Ptr' -> 'store int 0x12345678, Ptr'
   // FIXME: We shouldn't do this for TargetConstantFP's.
   // FIXME: move this to the DAG Combiner!  Note that we can't regress due
   // to phase ordering between legalized code and the dag combiner.  This
   // probably means that we need to integrate dag combiner and legalizer
   // together.
   // We generally can't do this one for long doubles.