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

Return int8Ty instead of i1 when function defined return type bool #32

Closed
redbopo opened this issue Mar 1, 2023 · 5 comments
Closed
Labels
bug Something isn't working

Comments

@redbopo
Copy link
Contributor

redbopo commented Mar 1, 2023

while trying to add LNot operation, notice that the type converter will make cir.bool translated to i8, I think i1 maybe more proper.
For example:

bool ulnot() {
  unsigned a = 0;
  return ~a;
}

./bin/clang -emit-cir t.cpp -o -

module attributes {cir.sob = #cir.signed_overflow_behavior<undefined>} {
  cir.func @_Z5ulnotv() -> !cir.bool {
    %0 = cir.alloca !cir.bool, cir.ptr <!cir.bool>, ["__retval"] {alignment = 1 : i64} loc(#loc2)
    %1 = cir.alloca i32, cir.ptr <i32>, ["a", init] {alignment = 4 : i64} loc(#loc9)
    %2 = cir.cst(0 : i32) : i32 loc(#loc4)
    cir.store %2, %1 : i32, cir.ptr <i32> loc(#loc9)
    %3 = cir.load %1 : cir.ptr <i32>, i32 loc(#loc5)
    %4 = cir.unary(not, %3) : i32, i32 loc(#loc6)
    %5 = cir.cast(int_to_bool, %4 : i32), !cir.bool loc(#loc10)
    cir.store %5, %0 : !cir.bool, cir.ptr <!cir.bool> loc(#loc11)
    %6 = cir.load %0 : cir.ptr <!cir.bool>, !cir.bool loc(#loc11)
    cir.return %6 : !cir.bool loc(#loc11)
  } loc(#loc8)
}

./bin/clang -emit-cir t.cpp -o - | ./bin/cir-tool -cir-to-llvm -o -

module {
  llvm.func @_Z5ulnotv() -> i8 {
    %0 = llvm.mlir.constant(1 : index) : i64
    %1 = llvm.alloca %0 x i8 {alignment = 1 : i64} : (i64) -> !llvm.ptr<i8>
    %2 = llvm.mlir.constant(1 : index) : i64
    %3 = llvm.alloca %2 x i32 {alignment = 4 : i64} : (i64) -> !llvm.ptr<i32>
    %4 = llvm.mlir.constant(0 : i32) : i32
    llvm.store %4, %3 : !llvm.ptr<i32>
    %5 = llvm.load %3 : !llvm.ptr<i32>
    %6 = llvm.mlir.constant(-1 : i32) : i32
    %7 = llvm.xor %6, %5  : i32
    %8 = llvm.mlir.constant(0 : i32) : i32
    %9 = llvm.icmp "ne" %7, %8 : i32
    %10 = llvm.zext %9 : i1 to i8
    llvm.store %10, %1 : !llvm.ptr<i8>
    %11 = llvm.load %1 : !llvm.ptr<i8>
    llvm.return %11 : i8
  }
}

@lanza
Copy link
Member

lanza commented Mar 1, 2023

For x86_64-unknown-linux-gnu Clang emits function type usages of bool to LLVM as an i1 but within the function emits i8s. So for example

bool foo(bool a) {
    return a;
}

yields:

define dso_local noundef zeroext i1 @_Z3foob(i1 noundef zeroext %a) #0 {
entry:
  %a.addr = alloca i8, align 1
  %frombool = zext i1 %a to i8
  store i8 %frombool, ptr %a.addr, align 1
  %0 = load i8, ptr %a.addr, align 1
  %tobool = trunc i8 %0 to i1
  ret i1 %tobool
}

So converting cir.bool to i8 is correct within the function but the functiontype is wrong. So it's not as straightforward as just changing the typeconverter to emit an i1. I don't recall off the top of my head how to fix it at the moment, though.

If you're motivated to take a look at this, one warning is that the constraint around the cir.bool -> i8 is very strongly constrained by numerous usages such as cir.if. I had to pepper a number of extra truncs and zexts to get everything in agreement with what clang does within a function. So I suggest looking specifically at the function type converter. That'll be my plan when I get around to looking at this.

@redbopo
Copy link
Contributor Author

redbopo commented Mar 2, 2023

Thanks for suggestion.
There is some difference between the examples we represent.

bool foo(bool a) {
    return a;
}

Using the bool variable a, will try to allocate an int8 type instruction.
If we change to unsigned or other type, it won't occur.

bool foo(unsigned a) {
    return a;
}

// ./bin/clang++ foo.cpp -S -emit-llvm -o -

define dso_local noundef zeroext i1 @_Z3fooj(i32 noundef %a) #0 {
entry:
  %a.addr = alloca i32, align 4
  store i32 %a, ptr %a.addr, align 4
  %0 = load i32, ptr %a.addr, align 4
  %tobool = icmp ne i32 %0, 0
  ret i1 %tobool
}


And the Clang CodeGen procedure of ConvertTypeForMem() which handles the formmer

}
/// ConvertTypeForMem - Convert type T into a llvm::Type. This differs from
/// ConvertType in that it is used to convert to the memory representation for
/// a type. For example, the scalar representation for _Bool is i1, but the
/// memory representation is usually i8 or i32, depending on the target.
llvm::Type *CodeGenTypes::ConvertTypeForMem(QualType T, bool ForBitField) {
if (T->isConstantMatrixType()) {
const Type *Ty = Context.getCanonicalType(T).getTypePtr();
const ConstantMatrixType *MT = cast<ConstantMatrixType>(Ty);
return llvm::ArrayType::get(ConvertType(MT->getElementType()),
MT->getNumRows() * MT->getNumColumns());

So I don't think using the i8 within the function is a correct method.

Meanwhile, here are two allocate instructiones in CIR while LLVM IR only one, seems like redundant in some where.

@lanza
Copy link
Member

lanza commented Mar 4, 2023

Using the bool variable a, will try to allocate an int8 type instruction.

Yup, that we know is wrong. And you seemed to have found out why by looking at CodeGenTypes::ConvertTypeForMem! I clearly missed the the (!T->isBitIntType() && R->isIntegerTy(1))) when adding this initially.

Thank you for pointing this out!

Meanwhile, here are two allocate instructiones in CIR while LLVM IR only one, seems like redundant in some where.

We explicitly always alloc the __retval. Clang seems to elide it in certain situations.

cir.func @_Z3foob(%arg0: !cir.bool) -> !cir.bool {
    %0 = cir.alloca !cir.bool, cir.ptr <!cir.bool>, ["a", init] {alignment = 1 : i64}
    %1 = cir.alloca !cir.bool, cir.ptr <!cir.bool>, ["__retval"] {alignment = 1 : i64}
    cir.store %arg0, %0 : !cir.bool, cir.ptr <!cir.bool>
    %2 = cir.load %0 : cir.ptr <!cir.bool>, !cir.bool
    cir.store %2, %1 : !cir.bool, cir.ptr <!cir.bool>
    %3 = cir.load %1 : cir.ptr <!cir.bool>, !cir.bool
    cir.return %3 : !cir.bool
  }

I'm not worried about this since it's clearly super trivial for an optimizer to know it can be removed.

@lanza
Copy link
Member

lanza commented Mar 5, 2023

This is actually a bit uglier than I thought. Clang shoehorns the bool to an i8 when it needs it and an i1 otherwise. So our representation has to keep track of what clang decided to do since this is effectively clang-codegen-ABI and we have to agree exactly with the ABI at the LLVMIR level if we want to be able to module link with CodeGen emitted IR files.

@smeenai
Copy link
Collaborator

smeenai commented Oct 11, 2024

I'm closing this out in favor of #480, since that was more recently updated.

@smeenai smeenai closed this as completed Oct 11, 2024
@smeenai smeenai closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants