[PATCH] DAGCombiner: Continue combining if FoldConstantArithmetic() fails.

[hfinkel at anl.gov]

Can you please upload this patch with full context (see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface for instructions).

> I think the rule here is: "Don't fold opaque constants into another expensive (according to TargetTransformInfo::getIntImmConst) Constant.

>  It's not a correctness issue but it diminishes the effects of the ConstantHoisting pass.


That does not seem right. Am I right that we only expect isOpaque to be true on TargetConstants, not regular Constants? My understanding is that opaque TargetConstants are a correctness issue (it is not really a cost issue, but rather that sometimes we can't change the constant because it has a special meaning, a larger constant might not be representable for that operation (like the offset on an indexed load or store), etc.).

FWIW, I feel like our current handling of isOpaque is laughably sparse, and we should either complete the job (by auditing, as you've done), or rip it out. Perhaps instead we should just consider all TargetConstants opaque.

That having been said, I'd like to add some additional safety here. Such as, in ConstantSDNode:

  const ConstantInt *getConstantIntValue() const { return Value; }

should become:

  const ConstantInt *getConstantIntValue(bool allowOpaque = false) const {
    assert((allowOpaque || !isOpaque()) && "Invalid access to opaque value");
    return Value;
  }

and likewise for the other interfaces. This way, by default, you can't even get an opaque value's numeric representation.


REPOSITORY
  rL LLVM

http://reviews.llvm.org/D6946

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/