[PATCH] D105853: [ISel] Expand saddsat and ssubsat via asr and xor

Tue Jul 13 00:33:42 PDT 2021

dmgreen added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/sadd_sat.ll:16
-; CHECK-NEXT:    mov w9, #2147483647
-; CHECK-NEXT:    cmp w8, #0 // =0
-; CHECK-NEXT:    cinv w8, w9, ge
----------------
efriedma wrote:
> We could save two instructions here without really changing the algorithm if we use the carry flag from the adds, i.e. `(UADDO LHS, RHS)`, instead of checking whether the result of the adds is negative.  That's the same number of instructions as your new sequence, but with a shorter critical path, so probably better?
Nice idea, although I'm not sure how much it would matter in practice. The thumb1 versions were my main target, they won't have been vectorized like most other architectures.

================
Comment at: llvm/test/CodeGen/X86/ssub_sat.ll:33
+; X64-NEXT:    subl %esi, %eax
+; X64-NEXT:    cmovol %ecx, %eax
 ; X64-NEXT:    retq
----------------
efriedma wrote:
> dmgreen wrote:
> > Now that I look again, some of the individual X86 tests have increased, although the total size of the files has gone down.
> > 
> > What do we usually do about such things? I could just custom lower this in ARM/AArch64, but it appears to be better in general.
> There are basically two options: use a target hook, or try to pick some reasonable default and write custom lowering for targets where the default is bad.  There's no great answer here.
> 
> Note that even the existing x86 code isn't really ideal: if we replace the addl with leal, we can save an instruction by merging the cmpl with the subl.
The files were all smaller, I hadn't noticed this was quite a common case though.

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

https://reviews.llvm.org/D105853