Re: [macppc 6.7-beta] clang backend error: "adde Constant" issue
> Date: Thu, 30 Apr 2020 12:58:32 -0400
> From: George Koehler
>
> On Wed, 29 Apr 2020 21:08:52 +0200 (CEST)
> Mark Kettenis wrote:
>
> > Upstream fixed this issue as well. Apparently only ADDE can't be
> > legalized (because it is "special") but ADDCARRY can. Do ypu want to
> > adjust your diff based on that information?
> >
> > Either way, ok kettenis@
>
> This adjusted diff changes only ADDE and not ADDCARRY. I expect it to
> work as well as my previous diff (Tue 28 Apr) on PowerPC, because
> PowerPC doesn't use ADDCARRY.
>
> After I built macppc clang with the previous diff, I did a successful
> "make build" in /usr/src. I'm not doing another "make build" with
> this adjusted diff, but I will check that some other stuff builds.
ok kettenis@
> Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> ===
> RCS file: /cvs/src/gnu/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp,v
> retrieving revision 1.1.1.8
> diff -u -p -r1.1.1.8 DAGCombiner.cpp
> --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp 23 Jun 2019 21:36:48 -
> 1.1.1.8
> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp 30 Apr 2020 16:18:31 -
> @@ -9904,9 +9904,11 @@ SDValue DAGCombiner::visitTRUNCATE(SDNod
>// (trunc adde(X, Y, Carry)) -> (adde trunc(X), trunc(Y), Carry)
>// (trunc addcarry(X, Y, Carry)) -> (addcarry trunc(X), trunc(Y), Carry)
>// When the adde's carry is not used.
> + // Don't make an illegal adde: LegalizeDAG can't expand nor promote it.
>if ((N0.getOpcode() == ISD::ADDE || N0.getOpcode() == ISD::ADDCARRY) &&
>N0.hasOneUse() && !N0.getNode()->hasAnyUseOfValue(1) &&
> - (!LegalOperations || TLI.isOperationLegal(N0.getOpcode(), VT))) {
> + ((!LegalOperations && N0.getOpcode() == ISD::ADDCARRY) ||
> + TLI.isOperationLegal(N0.getOpcode(), VT))) {
> SDLoc SL(N);
> auto X = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(0));
> auto Y = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(1));
>
Re: [macppc 6.7-beta] clang backend error: "adde Constant" issue
On Thu, Apr 30, 2020 at 12:58:32PM -0400, George Koehler wrote:
> On Wed, 29 Apr 2020 21:08:52 +0200 (CEST)
> Mark Kettenis wrote:
>
> > Upstream fixed this issue as well. Apparently only ADDE can't be
> > legalized (because it is "special") but ADDCARRY can. Do ypu want to
> > adjust your diff based on that information?
> >
> > Either way, ok kettenis@
>
> This adjusted diff changes only ADDE and not ADDCARRY. I expect it to
> work as well as my previous diff (Tue 28 Apr) on PowerPC, because
> PowerPC doesn't use ADDCARRY.
>
> After I built macppc clang with the previous diff, I did a successful
> "make build" in /usr/src. I'm not doing another "make build" with
> this adjusted diff, but I will check that some other stuff builds.
Looks good to me. ok mortimer@
>
> Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> ===
> RCS file: /cvs/src/gnu/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp,v
> retrieving revision 1.1.1.8
> diff -u -p -r1.1.1.8 DAGCombiner.cpp
> --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp 23 Jun 2019 21:36:48 -
> 1.1.1.8
> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp 30 Apr 2020 16:18:31 -
> @@ -9904,9 +9904,11 @@ SDValue DAGCombiner::visitTRUNCATE(SDNod
>// (trunc adde(X, Y, Carry)) -> (adde trunc(X), trunc(Y), Carry)
>// (trunc addcarry(X, Y, Carry)) -> (addcarry trunc(X), trunc(Y), Carry)
>// When the adde's carry is not used.
> + // Don't make an illegal adde: LegalizeDAG can't expand nor promote it.
>if ((N0.getOpcode() == ISD::ADDE || N0.getOpcode() == ISD::ADDCARRY) &&
>N0.hasOneUse() && !N0.getNode()->hasAnyUseOfValue(1) &&
> - (!LegalOperations || TLI.isOperationLegal(N0.getOpcode(), VT))) {
> + ((!LegalOperations && N0.getOpcode() == ISD::ADDCARRY) ||
> + TLI.isOperationLegal(N0.getOpcode(), VT))) {
> SDLoc SL(N);
> auto X = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(0));
> auto Y = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(1));
Re: [macppc 6.7-beta] clang backend error: "adde Constant" issue
On Wed, 29 Apr 2020 21:08:52 +0200 (CEST)
Mark Kettenis wrote:
> Upstream fixed this issue as well. Apparently only ADDE can't be
> legalized (because it is "special") but ADDCARRY can. Do ypu want to
> adjust your diff based on that information?
>
> Either way, ok kettenis@
This adjusted diff changes only ADDE and not ADDCARRY. I expect it to
work as well as my previous diff (Tue 28 Apr) on PowerPC, because
PowerPC doesn't use ADDCARRY.
After I built macppc clang with the previous diff, I did a successful
"make build" in /usr/src. I'm not doing another "make build" with
this adjusted diff, but I will check that some other stuff builds.
Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===
RCS file: /cvs/src/gnu/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp,v
retrieving revision 1.1.1.8
diff -u -p -r1.1.1.8 DAGCombiner.cpp
--- lib/CodeGen/SelectionDAG/DAGCombiner.cpp23 Jun 2019 21:36:48 -
1.1.1.8
+++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp30 Apr 2020 16:18:31 -
@@ -9904,9 +9904,11 @@ SDValue DAGCombiner::visitTRUNCATE(SDNod
// (trunc adde(X, Y, Carry)) -> (adde trunc(X), trunc(Y), Carry)
// (trunc addcarry(X, Y, Carry)) -> (addcarry trunc(X), trunc(Y), Carry)
// When the adde's carry is not used.
+ // Don't make an illegal adde: LegalizeDAG can't expand nor promote it.
if ((N0.getOpcode() == ISD::ADDE || N0.getOpcode() == ISD::ADDCARRY) &&
N0.hasOneUse() && !N0.getNode()->hasAnyUseOfValue(1) &&
- (!LegalOperations || TLI.isOperationLegal(N0.getOpcode(), VT))) {
+ ((!LegalOperations && N0.getOpcode() == ISD::ADDCARRY) ||
+ TLI.isOperationLegal(N0.getOpcode(), VT))) {
SDLoc SL(N);
auto X = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(0));
auto Y = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(1));
Re: [macppc 6.7-beta] clang backend error: "adde Constant" issue
> Date: Tue, 28 Apr 2020 01:31:12 -0400
> From: George Koehler
>
> Moving from bugs@ to tech@,
> because some people might miss a clang diff on bugs@.
>
> This diff modifies LLVM's DAGCombiner to skip an optimization if it
> would make an illegal ISD::ADDE node. This fixes fatal errors from
> powerpc clang when building ports net/libtorrent-rasterbar and
> devel/avr/binutils on PowerPC. The fatal errors look like,
>
> On Sun, 19 Apr 2020 17:26:49 +0200
> Charlene Wendling wrote:
>
> > fatal error: error in backend: Cannot select: 0x90a1c1b0: i1,glue = adde
> > Constant:i1<0>, Constant:i1<-1>, 0xa6a2b1b0:1
> > 0xa6a2b828: i1 = Constant<0>
> > 0x90a1c630: i1 = Constant<-1>
> > 0xa6a2b1b0: i32,glue = addc 0x90a1c3a8, Constant:i32<-1>
>
> An example to cause this error, given unsigned 32-bit *p, is
> if (*p > ((*p + 15ULL) & ~15ULL)) ...
>
> The SelectionDAG goes through phases. The DAG starts with an i64 add
> *p + 15ULL, but 32-bit PowerPC has no i64 ops, so the legalize-types
> phase converts the i64 add to an i32 addc/adde pair. The DAG keeps
> some i1 ops because clang -mcrbits (the default) enables i1 ops on
> condition register bits. DAGCombiner is optimizing the i1 ops when it
> truncates the i32 adde to an i1 adde, but i1 adde is an illegal op.
>
> Legalize-ops runs next, and would convert illegal ops to legal ops,
> but it can't fix i1 adde, because it has no case for ISD::ADDE.
> LegalizeIntegerTypes.cpp:1966 knows this, and refuses to insert an
> illegal adde, but DAGCombiner::visitTRUNCATE didn't know.
>
> This diff might affect other arches. I guess that Aarch64, ARM, MIPS,
> Sparc don't have integer ops smaller than i32, and X86 doesn't use
> ISD::ADDE, so this diff might not affect them.
>
> OK to commit?
Upstream fixed this issue as well. Apparently only ADDE can't be
legalized (because it is "special") but ADDCARRY can. Do ypu want to
adjust your diff based on that information?
Either way, ok kettenis@
> Index: lib/CodeGen//SelectionDAG/DAGCombiner.cpp
> ===
> RCS file: /cvs/src/gnu/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp,v
> retrieving revision 1.1.1.8
> diff -u -p -r1.1.1.8 DAGCombiner.cpp
> --- lib/CodeGen//SelectionDAG/DAGCombiner.cpp 23 Jun 2019 21:36:48 -
> 1.1.1.8
> +++ lib/CodeGen//SelectionDAG/DAGCombiner.cpp 27 Apr 2020 21:48:18 -
> @@ -9904,9 +9904,10 @@ SDValue DAGCombiner::visitTRUNCATE(SDNod
>// (trunc adde(X, Y, Carry)) -> (adde trunc(X), trunc(Y), Carry)
>// (trunc addcarry(X, Y, Carry)) -> (addcarry trunc(X), trunc(Y), Carry)
>// When the adde's carry is not used.
> + // Don't make an illegal adde: LegalizeDAG can't expand nor promote it.
>if ((N0.getOpcode() == ISD::ADDE || N0.getOpcode() == ISD::ADDCARRY) &&
>N0.hasOneUse() && !N0.getNode()->hasAnyUseOfValue(1) &&
> - (!LegalOperations || TLI.isOperationLegal(N0.getOpcode(), VT))) {
> + TLI.isOperationLegal(N0.getOpcode(), VT)) {
> SDLoc SL(N);
> auto X = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(0));
> auto Y = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(1));
>
>
Re: [macppc 6.7-beta] clang backend error: "adde Constant" issue
Moving from bugs@ to tech@,
because some people might miss a clang diff on bugs@.
This diff modifies LLVM's DAGCombiner to skip an optimization if it
would make an illegal ISD::ADDE node. This fixes fatal errors from
powerpc clang when building ports net/libtorrent-rasterbar and
devel/avr/binutils on PowerPC. The fatal errors look like,
On Sun, 19 Apr 2020 17:26:49 +0200
Charlene Wendling wrote:
> fatal error: error in backend: Cannot select: 0x90a1c1b0: i1,glue = adde
> Constant:i1<0>, Constant:i1<-1>, 0xa6a2b1b0:1
> 0xa6a2b828: i1 = Constant<0>
> 0x90a1c630: i1 = Constant<-1>
> 0xa6a2b1b0: i32,glue = addc 0x90a1c3a8, Constant:i32<-1>
An example to cause this error, given unsigned 32-bit *p, is
if (*p > ((*p + 15ULL) & ~15ULL)) ...
The SelectionDAG goes through phases. The DAG starts with an i64 add
*p + 15ULL, but 32-bit PowerPC has no i64 ops, so the legalize-types
phase converts the i64 add to an i32 addc/adde pair. The DAG keeps
some i1 ops because clang -mcrbits (the default) enables i1 ops on
condition register bits. DAGCombiner is optimizing the i1 ops when it
truncates the i32 adde to an i1 adde, but i1 adde is an illegal op.
Legalize-ops runs next, and would convert illegal ops to legal ops,
but it can't fix i1 adde, because it has no case for ISD::ADDE.
LegalizeIntegerTypes.cpp:1966 knows this, and refuses to insert an
illegal adde, but DAGCombiner::visitTRUNCATE didn't know.
This diff might affect other arches. I guess that Aarch64, ARM, MIPS,
Sparc don't have integer ops smaller than i32, and X86 doesn't use
ISD::ADDE, so this diff might not affect them.
OK to commit?
Index: lib/CodeGen//SelectionDAG/DAGCombiner.cpp
===
RCS file: /cvs/src/gnu/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp,v
retrieving revision 1.1.1.8
diff -u -p -r1.1.1.8 DAGCombiner.cpp
--- lib/CodeGen//SelectionDAG/DAGCombiner.cpp 23 Jun 2019 21:36:48 -
1.1.1.8
+++ lib/CodeGen//SelectionDAG/DAGCombiner.cpp 27 Apr 2020 21:48:18 -
@@ -9904,9 +9904,10 @@ SDValue DAGCombiner::visitTRUNCATE(SDNod
// (trunc adde(X, Y, Carry)) -> (adde trunc(X), trunc(Y), Carry)
// (trunc addcarry(X, Y, Carry)) -> (addcarry trunc(X), trunc(Y), Carry)
// When the adde's carry is not used.
+ // Don't make an illegal adde: LegalizeDAG can't expand nor promote it.
if ((N0.getOpcode() == ISD::ADDE || N0.getOpcode() == ISD::ADDCARRY) &&
N0.hasOneUse() && !N0.getNode()->hasAnyUseOfValue(1) &&
- (!LegalOperations || TLI.isOperationLegal(N0.getOpcode(), VT))) {
+ TLI.isOperationLegal(N0.getOpcode(), VT)) {
SDLoc SL(N);
auto X = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(0));
auto Y = DAG.getNode(ISD::TRUNCATE, SL, VT, N0.getOperand(1));
