[PATCH] D150769: [SelectionDAG][computeKnownBits]: Move ISD::ADD/ISD::SUB into their own cases

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D150769#4371343 <https://reviews.llvm.org/D150769#4371343>, @nikic wrote:

> @alexfh Thanks for the report, I've reverted the change for now.
>
> This patch has exposed a pre-existing miscompile in integer legalization. The relevant pre-legalization DAG is:
>
>   t0: ch,glue = EntryToken
>                       t2: i64,ch = CopyFromReg t0, Register:i64 %0
>                     t5: i64 = srl t2, Constant:i8<1>
>                   t6: i32 = truncate t5
>                 t8: i32 = add t6, Constant:i32<1>
>               t9: i63 = zero_extend t8
>             t11: i63 = add nsw t9, Constant:i63<-1>
>           t12: i64 = zero_extend t11
>         t13: i64 = add nuw t12, Constant:i64<1>
>       t25: i64 = and t13, Constant:i64<-16>
>     t21: i64 = or t25, Constant:i64<1>
>   t20: ch = CopyToReg t0, Register:i64 %2, t21
>
> The type legalized DAG is:
>
>   t0: ch,glue = EntryToken
>                       t2: i64,ch = CopyFromReg t0, Register:i64 %0
>                     t5: i64 = srl t2, Constant:i8<1>
>                   t6: i32 = truncate t5
>                 t8: i32 = add t6, Constant:i32<1>
>               t27: i64 = zero_extend t8
>             t28: i64 = add nsw t27, Constant:i64<9223372036854775807>
>           t29: i64 = and t28, Constant:i64<9223372036854775807>
>         t13: i64 = add nuw t29, Constant:i64<1>
>       t25: i64 = and t13, Constant:i64<-16>
>     t21: i64 = or t25, Constant:i64<1>
>   t20: ch = CopyToReg t0, Register:i64 %2, t21
>
> Note that the `nsw` from the `add -1` was incorrectly transferred to the `add INT_MAX`. Legalization of adds works on any-ext promoted operands, but then https://github.com/llvm/llvm-project/blob/89242ed6a28b8227890af7691e573bfc4dc55a91/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp#LL718C22-L718C22 will just blindly copy (!!!) all flags from the old node to the new one. It would be valid to do this for add nsw if we used sexted operands, but that's not what we do. This is embarrassingly bad.
>
> The good news is that removing that flag copy doesn't cause catastrophic test fallout: https://gist.github.com/nikic/5f1d3b0c78f646f74c84aac68a40de76 So we probably just need to move that into some of the individual promotion methods where it is safe.

We just stumbled upon the same thing downstream and I also tracked it down to DAGTypeLegalizer::SetPromotedInteger.

I think it would be nice to at least stop copying the nsw/nuw flags in DAGTypeLegalizer::SetPromotedInteger as a first step (original intent seem to have been to copy fast-math-flags, no idea if that always is safe, maybe not?).

And then we could try to add back nsw/nuw propagation on a more fine-grained per-operation level. When for example dealing with an ADD operation we need to figure out if it is better to promote operands by sign-extend to allow nsw to remain, or if it is better to promote without sext and drop the flag. Sometimes the promotion would be by sign-extension anyway (I was looking at test/CodeGen/AArch64/arm64-vhadd.ll) and then I guess we want to keep the nsw flag in that specific case.

Has anyone already written a github issue for following up on the DAGTypeLegalizer::SetPromotedInteger bug?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150769/new/

https://reviews.llvm.org/D150769