Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sign extension not properly propagated to users #387

Open
j2kun opened this issue Feb 27, 2024 · 2 comments
Open

Sign extension not properly propagated to users #387

j2kun opened this issue Feb 27, 2024 · 2 comments

Comments

@j2kun
Copy link
Contributor

j2kun commented Feb 27, 2024

Given the following C++ code:

short isqrt(short num) {
  short res = 0;
  short bit = 1 << 14;  // ((unsigned) INT16_MAX + 1) / 2.

  for (int i = 0; i < 8; ++i) {
    if (num >= res + bit) {
      num -= res + bit;
      res = (res >> 1) + bit;
    } else {
      res >>= 1;
    }
    bit >>= 2;
  }
  return res;
}

Running cgeist at 4b04755 as follows:

bin/cgeist \
  '-function=*' \
  -raise-scf-to-affine \
  --memref-fullrank \
  -S \
  -O0 \
  sqrt.cc

Produces

loc("/path/to/sqrt.cc":25:11): error: 'arith.subi' op requires the same type for all operands and results
"builtin.module"() ({
  "func.func"() <{function_type = (i16) -> i16, sym_name = "_Z5isqrts"}> ({
  ^bb0(%arg0: i16):
    %0 = "arith.constant"() <{value = 16384 : i16}> : () -> i16
    %1 = "arith.constant"() <{value = 2 : i16}> : () -> i16
    %2 = "arith.constant"() <{value = 1 : i16}> : () -> i16
    %3 = "arith.constant"() <{value = 1 : i32}> : () -> i32
    %4 = "arith.constant"() <{value = 0 : i16}> : () -> i16
    %5:3 = "affine.for"(%0, %4, %arg0) ({
    ^bb0(%arg1: index, %arg2: i16, %arg3: i16, %arg4: i16):
      %6 = "arith.extsi"(%arg4) : (i16) -> i32
      %7 = "arith.extsi"(%arg3) : (i16) -> i32
      %8 = "arith.extsi"(%arg2) : (i16) -> i32
      %9 = "arith.addi"(%7, %8) : (i32, i32) -> i32
      %10 = "arith.cmpi"(%6, %9) <{predicate = 5 : i64}> : (i32, i32) -> i1
      %11:2 = "scf.if"(%10) ({
        %13 = "arith.subi"(%arg4, %9) : (i16, i32) -> i16
        %14 = "arith.shrsi"(%7, %3) : (i32, i32) -> i32
        %15 = "arith.addi"(%14, %8) : (i32, i32) -> i32
        %16 = "arith.trunci"(%15) : (i32) -> i16
        "scf.yield"(%16, %13) : (i16, i16) -> ()
      }, {
        %13 = "arith.shrsi"(%arg3, %2) : (i16, i16) -> i16
        "scf.yield"(%13, %arg4) : (i16, i16) -> ()
      }) : (i1) -> (i16, i16)
      %12 = "arith.shrsi"(%arg2, %1) : (i16, i16) -> i16
      "affine.yield"(%12, %11#0, %11#1) : (i16, i16, i16) -> ()
    }) {lower_bound = affine_map<() -> (0)>, step = 1 : index, upper_bound = affine_map<() -> (8)>} : (i16, i16, i16) -> (i16, i16, i16)
    "func.return"(%5#1) : (i16) -> ()
  }) {llvm.linkage = #llvm.linkage<external>} : () -> ()
}) {<lots of attributes>} : () -> ()

It looks like the issue is that

%13 = "arith.subi"(%arg4, %9) : (i16, i32) -> i16

Is still using %arg4 even though it was sign extended in an earlier line

%6 = "arith.extsi"(%arg4) : (i16) -> i32
@j2kun
Copy link
Contributor Author

j2kun commented Apr 3, 2024

@ivanradanov I'd like to fix this issue along with #388. Is the project actively soliciting contributions? Is there bandwidth to review my fixes? It's not clear to me who is the official maintainer to contact, so please redirect me if it's not you.

@wsmoses
Copy link
Member

wsmoses commented Apr 3, 2024

Go for it patches welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants