[PATCH] D123014: [X86][FastISel] Remove with.overflow + select fold (PR54369)

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D123014#3435763 <https://reviews.llvm.org/D123014#3435763>, @pengfei wrote:

> In D123014#3435488 <https://reviews.llvm.org/D123014#3435488>, @RKSimon wrote:
>
>> In D123014#3429542 <https://reviews.llvm.org/D123014#3429542>, @nikic wrote:
>>
>>> @RKSimon Here's how dropping it entirely looks like: https://gist.github.com/nikic/9e59e8e7b635a54f0d75aad89b2632d6
>>
>> I think I prefer getting rid of it entirely, but I'd accept just this patch if others want to keep some eflags folding behaviour.
>>
>> @pengfei @craig.topper any thoughts?
>
> If I read the patches correct, this patch and further removing the whole folding are just for theoretical potential issues but we cannot think out for now, right?
> Given we don't do much optimizations with FastISel (which means it's rare we meet another fail in future), I prefer to D122825 <https://reviews.llvm.org/D122825> (maybe with comments for future information). Furthermore, I think we can even check if the constant is 0.

You can't check if the constant is 0. It could be a ConstantExpr that includes a use of a 0. The ConstantExpr will get expanded to individual instructions and that might create a mov of 0. It could also expand to an expression that includes an `add` or other instruction which will also clobber the flags. For example, this test case is broken today because it creates an add instruction between the overflow add and the cmov.

  @a = global i32 0, align 4
  
  define i64 @adder(i64 %lhs, i64 %rhs) {
          %res = call { i64, i1 } @llvm.sadd.with.overflow.i64(i64 %lhs, i64 %rhs)
          %errorbit = extractvalue { i64, i1 } %res, 1
          %errorval = select i1 %errorbit, i64 148, i64 add (i64 ptrtoint (i32* @a to i64), i64 5)
          ret i64 %errorval
  }
  
  declare { i64, i1 } @llvm.sadd.with.overflow.i64(i64 %a, i64 %b)


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

https://reviews.llvm.org/D123014