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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150769