[PATCH] D106139: [AMDGPU] Combine srl of add that intends to get the carry of the add as addcarry

[Abinav Puthan Purayil via Phabricator via llvm-commits]

abinavpp added a comment.

  Before I address the comments on the code, test-case, coverage etc. we need to
  be on the same page on where and how this should be done. I can see 2
  approaches, but I'm unable to decide between the 2:
  
  Approach A
  ----------
  Continue with the current approach, which transforms the DAG:
  t16: i64 = add t9, t15
    ...
    t15: i64 = srl t12, Constant:i32<32>
      t12: i64 = add t10, t11
        t10: i64 = zero_extend t2
          t2: i32,ch = CopyFromReg t0, Register:i32 %0
        t11: i64 = zero_extend t4
          t4: i32,ch = CopyFromReg t0, Register:i32 %1
  
  To:
  t16: i64 = add t9, t40:1
    ...
    t40: i32,i64 = addcarry t2, t4, Constant:i64<0>
      t2: i32,ch = CopyFromReg t0, Register:i32 %0
      t4: i32,ch = CopyFromReg t0, Register:i32 %1
  
  I'm not sure what we can do from the DAG-combiner's perspective to emit
  v_cndmask_b32. But instead, we can fix the expansion (in this case,
  V_ADD_U64_PSEUDO) to handle "carry-out as a source" with v_cndmask_b32.
  
  Approach B
  ----------
  Go with @foad's suggestion of using inst-combine and transform the input to:
  define i64 @uaddCarry(i32 %a, i32 %b, i64 %c) {
  entry:
    %uaddCarry = call {i32, i1} @llvm.uadd.with.overflow.i32(i32 %a, i32 %b)
    %uadd = extractvalue {i32, i1} %uaddCarry, 0
    %carry = extractvalue {i32, i1} %uaddCarry, 1
    %carry.zext = zext i1 %carry to i64
    %add = add i64 %c, %carry.zext
    ret i64 %add
  }
  declare {i32, i1} @llvm.uadd.with.overflow.i32(i32 %a, i32 %b)
  
  which would be lowered to:
  v_add_co_u32_e32 v0, vcc, v0, v1
  v_cndmask_b32_e64 v0, 0, 1, vcc
  v_add_co_u32_e32 v0, vcc, v2, v0
  v_addc_co_u32_e32 v1, vcc, 0, v3, vcc
  
  We can keep this transformation target independent, but which approach should we
  go with: Approach A (DAG-combine) or Approach B (inst-combine)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106139