[PATCH] D40701: [ARM][DAG] Reenable post-legalize store merge

[Nirav Dave via Phabricator via llvm-commits]

niravd added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12712
+                        .zextOrTrunc(ElementSizeBits)
+                        .zextOrTrunc(SizeInBits);
       } else {
----------------
eastig wrote:
> efriedma wrote:
> > eastig wrote:
> > > efriedma wrote:
> > > > This isn't what a "truncating" store means for floating-point values.  It converts the value using ISD::FP_ROUND.  (We use nodes like this to model something like an x87 fstps; not sure if any other target uses it.)
> > > Does it mean such stores should not be merged?
> > We can still merge them... we just have to call APFloat::convert() to narrow the value to the right size, as opposed to an integer truncate.  Or I guess we could just blacklist float truncstores.
> Hmm, the original code was
> 
> ```
> StoreInt |= C->getValueAPF().bitcastToAPInt().zext(SizeInBits);
> ```
> 
> In r303802 Nirav changed it into the version on left. The commit comment is
> 
> > [DAG] Prevent crashes when merging constant stores with high-bit set. NFC.
> 
> According to your comment truncating can destroy the value. So the commit was not NFC.
Eli:

Is that right? 

Looking at LegalizeDAG, when we expand a TruncStore we do it as a store of a truncate which IIUC is strictly bitvector-level interpretation. 




Repository:
  rL LLVM

https://reviews.llvm.org/D40701