[PATCH] D28284: [Legalizer] Fix fp-to-uint to fp-tosint promotion assertion.

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:436
+  return DAG.getNode(N->getOpcode() == ISD::FP_TO_UINT ?
                      ISD::AssertZext : ISD::AssertSext, dl, NVT, Res,
                      DAG.getValueType(N->getValueType(0).getScalarType()));
----------------
efriedma wrote:
> Not a comment about your patch, exactly, but I don't think the assertion here is safe; AssertZext(UNDEF) can lead to undefined behavior.  For example, LOAD(ADD(global_array, AND(SomeUndefValue, 255))) produces undef, assuming global_array has at least 256 elements.  However, LOAD(ADD(global_array, AND(AssertZext(SomeUndefValue, i8), 255))) simplifies to LOAD(ADD(global_array, AssertZext(SomeUndefValue, i8))), which has undefined behavior.
I agree, and I think being undefined in such a scenario matches part of the fptoui/fptosi contract:

  If the value cannot fit in ty2, the results are undefined.

Since the value undef cannot fit in "ty2", the integer type, we allow undefined behavior here.


https://reviews.llvm.org/D28284