This is an archive of the discontinued LLVM Phabricator instance.

DAGCombiner: Continue combining if FoldConstantArithmetic() fails.
ClosedPublic

Authored by MatzeB on Jan 12 2015, 7:05 PM.

Details

Summary

DAG.FoldConstantArithmetic() can fail even though both operands are
Constants if OpaqueConstants are involved. Continue trying other combine
possibilities in this case.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 18068.Jan 12 2015, 7:05 PM
MatzeB retitled this revision from to DAGCombiner: Continue combining if FoldConstantArithmetic() fails..
MatzeB updated this object.
MatzeB edited the test plan for this revision. (Show Details)
MatzeB added a reviewer: resistor.
MatzeB added subscribers: qcolombet, ab, Unknown Object (MLST).

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...

ab added inline comments.Jan 12 2015, 8:01 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1632

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).

MatzeB updated this revision to Diff 18111.Jan 13 2015, 1:53 PM

Simplify by using condition variables.

ab accepted this revision.Jan 19 2015, 5:05 PM
ab added a reviewer: ab.

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

This revision is now accepted and ready to land.Jan 19 2015, 5:05 PM
MatzeB updated this revision to Diff 19269.Feb 3 2015, 2:30 PM
MatzeB edited edge metadata.
MatzeB set the repository for this revision to rL LLVM.

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.

ab requested changes to this revision.Feb 12 2015, 11:09 AM
ab edited edge metadata.

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.

This revision now requires changes to proceed.Feb 12 2015, 11:09 AM

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
ab resigned from this revision.Feb 24 2015, 10:32 AM
ab edited reviewers, added: ributzka; removed: ab.

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

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
ab added a comment.Feb 24 2015, 11:25 AM

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

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
RKSimon added a subscriber: RKSimon.May 7 2015, 2:55 AM

Would it be tidier to set the N0C / N1C values to null if they are opaque instead of adding isOpaque() tests everywhere?

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.

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.).

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...

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.).

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...

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)?

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.

MatzeB updated this revision to Diff 26032.May 18 2015, 7:45 PM

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.

hfinkel accepted this revision.May 20 2015, 9:41 AM
hfinkel added a reviewer: hfinkel.
hfinkel added inline comments.
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.

This revision is now accepted and ready to land.May 20 2015, 9:41 AM
MatzeB closed this revision.May 29 2015, 2:37 PM

This landed in r237822. Closing manually as "Differential Revision: XXX" was not the last line of the commit message.