Add clang CIR codegen support for C/C++ unary operators

[bcardosolopes]

No description provided.

[cmarcelo]

I'm working on this one.

[j2kun]

Is anyone still working on this? I ran into

clangir/clang/lib/CIR/CodeGen/CIRGenExpr.cpp

Lines 712 to 714 in 855f623

	case UO_PreInc:
	case UO_PreDec: {
	assert(0 && "not implemented");

when compiling

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;
}

The first failure was the loop pre-increment as an expression. I'd be happy to add it, though it seems from this commit that there are quite a few cases to deal with... maybe if someone could provide me a quick outline I could get started?

[bcardosolopes]

Hi @j2kun, thanks for the interest working on this.

We try to match the skeleton as much as possible with the LLVM's codegen. So in this case, you need something like this from CGExpr.cpp

You can put a llvm_unreachable for the complex case, and in the scalar just call buildScalarPrePostIncDec (instead of EmitScalarPrePostIncDec). Same for buildLValue as opposed to EmitLValue.

[bcardosolopes]

This one is a definitely a good first issue @j2kun, @cmarcelo isn't working on the rest of this atm.

[j2kun]

Will take a crack at it. Is there a reason the cases for PostInc/Dec aren't in the same switch?

[j2kun]

I'm a little bit confused about how the ScalarExprEmitter is organized. It's in an anonymous namespace (and only declares its class name) making its methods inaccessible outside of its cpp file.

clangir/clang/lib/CIR/CodeGen/CIRGenFunction.h

Lines 44 to 47 in 62c31af

	namespace {
	class ScalarExprEmitter;
	class AggExprEmitter;
	}

Is there a reason for this? I see there's a friend class there, but it's not clear to me how that can be used if the class is only forward declared... Wouldn't it instead be more appropriate to have a header file for CIRGenExprScalar, declare its methods as usual, etc?

[bcardosolopes]

making its methods inaccessible outside of its cpp file.

That's because the methods aren't supposed to be used outside the cpp file. Some of these could be better, but we don't want to go to far off the skeleton from LLVM codegen, since it makes it easier to compare both when adding more CIRGen.

Follow the existing pattern and/or mimic how LLVM codegen does it and you should be good, we haven't had any problems so far.

[j2kun]

I see where my confusion was now.

In the LLVM codegen, the externally accessible EmitScalarPrePostIncDec is declared in CodeGenFunction.h, but implemented in (the public part of) CGExprScalar.cpp (not CodeGenFunction.cpp). Will mirror that.

[j2kun]

PTAL at #73 at your leisure. Thanks for the help!

[dkolsen-pgi]

What, if anything, is left to be done, for this issue? I tested all the unary operators on built-in types, and they all seem to work.

[bcardosolopes]

Right, for builtin types I believe it's mostly done - not all possible original codegen paths are yet supported though, if you search for "unary" in clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp you can spot those around (few examples are bitfield/atomic). I believe there are better first issues, let me tag a few more.

I'm gonna close this already since any follow up could be it's own issue if necessary.