Page MenuHomePhabricator

[DAGCombine] Modification of visitBR_CC
Needs ReviewPublic

Authored by JongwonLee on Sep 7 2016, 7:37 PM.

Details

Reviewers
bryant
spatel
Summary

visitBR_CC is modified to support below IR change.

%2 = add %1, #const
br_cc cond %2, #const
-->
br_cc cond %1, 0

Diff Detail

Event Timeline

JongwonLee updated this revision to Diff 70636.Sep 7 2016, 7:37 PM
JongwonLee retitled this revision from to [DAGCombine] Modification of visitBR_CC.
JongwonLee updated this object.
JongwonLee added reviewers: RKSimon, bryant, spatel.
JongwonLee added a subscriber: llvm-commits.
bryant added inline comments.Sep 8 2016, 6:03 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9673

could this be generalized further? i.e., (br_cc cc, (add X, c0), c1, bb) => (br_cc cc, X, (c1 - c0), bb), assuming that c1 >= c0. and then generalize that to other binary ops?

9687

EVT::getScalarSizeInBits. also, isn't it already the case that VT(AddRHS) == VT(AddLHS) == VT(CondLHSNode) (because of the implied constraint on add operands) and VT(CondLHSNode) == VT(CondRHS) (because of br_cc's constraint)?

9695

not so sure that this is the appropriate place for debug output.

test/CodeGen/AArch64/DAGCombine-optimize-brcc.ll
46

could this test case be simplified?

spatel edited edge metadata.Sep 8 2016, 6:28 AM

%2 = add %1, #const
br_cc cond %2, #const
-->
br_cc cond %1, 0

I don't think this transform is safe unless you have 'nsw' or 'nuw' on the add.

bryant edited edge metadata.Sep 9 2016, 2:11 AM

%2 = add %1, #const
br_cc cond %2, #const
-->
br_cc cond %1, 0

I don't think this transform is safe unless you have 'nsw' or 'nuw' on the add.

am i missing something obvious? if nsw/nuw are present, then %2 could potentially be undef, and the transform would elide this case and alter semantics. so wouldn't it be unsafe only if the flags are present?

%2 = add %1, #const
br_cc cond %2, #const
-->
br_cc cond %1, 0

I don't think this transform is safe unless you have 'nsw' or 'nuw' on the add.

am i missing something obvious? if nsw/nuw are present, then %2 could potentially be undef, and the transform would elide this case and alter semantics. so wouldn't it be unsafe only if the flags are present?

The wrapping flags are what allow us to guarantee that the transform does not alter semantics.

Consider the example with these values:

%1 is i8 255 (unsigned max)
%2 = add i8 %1, 1
br_cc ugt %2, 1

Wrapping is allowed; there is no undef.
The branch condition is:

ugt 0, 1 --> false.

If we make the transform suggested by this patch, the branch condition is:

ugt 255, 0  --> true

If the add has 'nuw', this case cannot occur. Otherwise, we have poison:
http://llvm.org/docs/LangRef.html#poisonvalues

Note that the semantics of DAG instructions are not actually specified/defined anywhere AFAIK (if they are, I'd love to know where!). I'm assuming they are the same as LLVM IR in this case because we can and do pass nsw/nuw flags from IR instructions to DAG nodes.

Please take a look at how this transform is handled for signed and unsigned compares in InstCombineCompares.cpp. I assume the reason we need this fold repeated as a DAG combine is that the pattern does not emerge until it's too late to be handled by InstCombine?

cc'ing Sanjoy and David in case I have any or all of this wrong. :)

Note: D24700 is proposing to enhance InstCombine's folding for a similar pattern.

RKSimon resigned from this revision.Jan 6 2017, 6:11 AM
RKSimon removed a reviewer: RKSimon.
RKSimon added a subscriber: RKSimon.
spatel resigned from this revision.Sep 26 2017, 3:52 PM