[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

> On Feb 12, 2015, at 11:09 AM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:
> 
> Peppering "isOpaque" checks in the remaining combines seems awfully brittle.
> 
> Basically, folding to constants isn't OK, but folding to undef is, correct?  How about flipping the check then:  do all the combines that are valid on Opaque constants first (-> undef), and then bail out if the constant is Opaque but we couldn't do anything.
> 
> 
> REPOSITORY
>  rL LLVM
> 
> http://reviews.llvm.org/D6946
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits