[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

[Sanjay Patel via Phabricator via cfe-commits]

spatel added a comment.

In D87188#2281957 <https://reviews.llvm.org/D87188#2281957>, @sanwou01 wrote:

> I know this has already been reverted but just FYI that I've bisected a ~2% regression in SPEC2017 x264_r on AArch64 to this commit. Presumably this is due to the extra unrolling / cost modelling issue already mentioned?

That would be my guess. I'm not familiar with the unroller or inliner, but I don't even see them using the cost model at 1st glance. Are they purely counting IR instructions?

If I missed the cost model calls, the cost model is still not correct for `code-size`:

  $ cat cost.ll
  declare i32        @llvm.abs.i32(i32, i1)
  define i32 @abs(i32 %x, i32 %y) {
    %I32 = call i32 @llvm.abs.i32(i32 %x, i1 false)
    %negx = sub i32 0, %x
    %cmp = icmp sgt i32 %x, -1
    %sel = select i1 %cmp, i32 %x, i32 %negx 
    ret i32 %I32
  }
  
  $ opt  -cost-model -cost-kind=code-size -analyze -mtriple=aarch64--  cost.ll 
  Printing analysis 'Cost Model Analysis' for function 'abs':
  Cost Model: Found an estimated cost of 1 for instruction:   %I32 = call i32 @llvm.abs.i32(i32 %x, i1 false)
  Cost Model: Found an estimated cost of 1 for instruction:   %negx = sub i32 0, %x
  Cost Model: Found an estimated cost of 1 for instruction:   %cmp = icmp sgt i32 %x, -1
  Cost Model: Found an estimated cost of 1 for instruction:   %sel = select i1 %cmp, i32 %x, i32 %negx

So we're assuming the size cost is either 1 or 3 depending on how we encoded abs() in IR, but neither is correct for AArch64 IIUC:

  $ llc -o - -mtriple=aarch64-- cost.ll 
  	cmp	w0, #0                          // =0
  	cneg	w0, w0, mi
  	ret


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87188