DAG.FoldConstantArithmetic() can fail even though both operands are
Constants if OpaqueConstants are involved. Continue trying other combine
possibilities in this case.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
Is there a way to create a proper llvm-lit test for this? I assume relying on -debug-only=isel in the RUN line is not a good idea.
The test in http://reviews.llvm.org/D6940 works but it is more of an accident that aarch64 fails on non properly combined code...
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1572 | How about: if (SDValue Folded = ...) return Folded; Or going further, you could remove the N0C && N1C check, since FoldConstantArithmetic also does it (a sliver more wasteful though). | |
Comment Actions
LGTM.
I'm okay with no test if it's tricky to expose, but maybe we can: what happens now on that AArch64 testcase? Specifically, is there a difference between just this patch, and this patch + the assert fix in D6940?
-Ahmed
Comment Actions
New version: Turns out if you continue combining in the presence of OpaqueConstants you have to be very carefull to not accidentally fold them into non-Opaque ones during a combine. This patch carefully finds these cases now (I basically looked at all places that create new target constants and checked whether they would be harmfully replace OpaqueConstants with new ones). I did extensive x86 test-suite runs and couldn't find any performance differences.
Comment Actions
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.
Comment Actions
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
Comment Actions
Fair enough, reordering the optimizations isn't a solution. Looking through old commits, I see there are a few that do exactly this: add isOpaque checks around DAG combines.
Having a separate node type for opaque constants does sound less brittle; any idea of what would be involved?
-Ahmed
Comment Actions
That would obviously involve changing all targets to recognize that new node type, in all sorts of different addressing modes. I also don't know whether all of this was already discussed when OpaqueConstants were introduced. Anyway I just wanted to fix a few bugs here and not rewrite all this stuff, so this patch won't get a LGTM unless it's a rewrite of the whole OpaqueConstants concepts?
- Matthias
Comment Actions
I think there are two parts here:
- checking the return value of FoldConstantArithmetic: that LGTM, though tests would of course be appreciated, if possible.
- disabling some optimizations on Opaque constants: that, I'll let Juergen comment on. I'm opposed to it because of the brittleness, but pragmatically (and there's precedent) that's a reasonable solution.
For your assert failure, I'm fine with just fixing that for now (that would be r230355, correct?).
-Ahmed
Comment Actions
I can't commit the first part without the second because it leads to the DAGCombiner folding many opaque constants back to normal constants breaking lit tests and generally defeating the intention of ContantHoisting pass.
- Matthias
Comment Actions
Would it be tidier to set the N0C / N1C values to null if they are opaque instead of adding isOpaque() tests everywhere?
Comment Actions
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.
Comment Actions
No, isOpaque is not about TargetConstants but normal ConstantSDNodes which are equivalent to IR Constants. Opaque constants are created in IR by Transforms/Scalar/ConstantHoisting.cpp with the convention that a Constant that is immediately bitcasted to the same type is an opaque constant. The trick here is not to undo the work from that pass while at the same time folding as much as possible...
Comment Actions
Ah, indeed. Shouldn't the rule be: Don't fold an opaque constant into another constant unless he result is cheap or free (in the getIntImmConst sense)?
Comment Actions
Ah, indeed. Shouldn't the rule be: Don't fold an opaque constant into another constant unless he result is cheap or free (in the getIntImmConst sense)?
Yes. In fact I did not bother to guard a few combines that produce a zero result, though admittedly the code does not check whether zero is an expensive constant - but I can't imagine that to be the case on any target.
Anyway I'm currently adapting the patch to ToT and will try out RKSimons suggestion which will probably simplify this a lot.
Comment Actions
New revision adapted to ToT and with many explicite isOpaque() checks avoided by introducing a cast operation that returns nullptr if a node is a non-opaque constant.
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 1600 | Our general convention seems to be to name functions like this 'getAs<Name>', not 'as<Name>', so I think getAsNonOpaqueConstant would be better. Otherwise, LGTM. | |
Comment Actions
This landed in r237822. Closing manually as "Differential Revision: XXX" was not the last line of the commit message.
Our general convention seems to be to name functions like this 'getAs<Name>', not 'as<Name>', so I think getAsNonOpaqueConstant would be better.
Otherwise, LGTM.