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

[Matthias Braun]

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.

It is indeed very brittle, but I am not convinced that reordering the optimization rules is the way to go. 
There are many other reasons on how the code/checks in a combiner should be order: What makes sense for a human reader or ordering the conditions in a way that the ones that are more likely to fail come first.
Letting a minor detail like Opaque Constants dictate the ordering feels very wrong to me. If we want to push hard to avoid accidentaly transforming OpaqueConstants back to normal ones then the way to go would be not using ISD::Constant for them anymore but to introduce a new opcode, so people need to explicitely check for them before transforming them. In the current design of Opaque being an attribute to ISD::Constant sprinkling the code with isOpaque() is a logical consequence IMO... It's not like drastic things happen we forget some isOpaque() checks anyway, it's not a correctness issue just a minor performance thing.

- Matthias


REPOSITORY
  rL LLVM

http://reviews.llvm.org/D6946

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