Skip to content

[CIR] Upstream support for promoted types with unary plus/minus #133829

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 3, 2025
Merged

[CIR] Upstream support for promoted types with unary plus/minus #133829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,15 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
llvm_unreachable("negation for the given type is NYI");
}

// TODO: split this to createFPExt/createFPTrunc when we have dedicated cast
// operations.
mlir::Value createFloatingCast(mlir::Value v, mlir::Type destType) {
assert(!cir::MissingFeatures::fpConstraints());

return create<cir::CastOp>(v.getLoc(), destType, cir::CastKind::floating,
v);
}

mlir::Value createFSub(mlir::Location loc, mlir::Value lhs, mlir::Value rhs) {
assert(!cir::MissingFeatures::metaDataNode());
assert(!cir::MissingFeatures::fpConstraints());
Expand Down
38 changes: 17 additions & 21 deletions clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,11 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
//===--------------------------------------------------------------------===//

mlir::Value emitPromotedValue(mlir::Value result, QualType promotionType) {
cgf.cgm.errorNYI(result.getLoc(), "floating cast for promoted value");
return {};
return builder.createFloatingCast(result, cgf.convertType(promotionType));
}

mlir::Value emitUnPromotedValue(mlir::Value result, QualType exprType) {
cgf.cgm.errorNYI(result.getLoc(), "floating cast for unpromoted value");
return {};
return builder.createFloatingCast(result, cgf.convertType(exprType));
}

mlir::Value emitPromoted(const Expr *e, QualType promotionType);
Expand Down Expand Up @@ -446,37 +444,35 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
llvm_unreachable("Unexpected signed overflow behavior kind");
}

mlir::Value VisitUnaryPlus(const UnaryOperator *e,
QualType promotionType = QualType()) {
if (!promotionType.isNull())
cgf.cgm.errorNYI(e->getSourceRange(), "VisitUnaryPlus: promotionType");
assert(!cir::MissingFeatures::opUnaryPromotionType());
mlir::Value result = emitUnaryPlusOrMinus(e, cir::UnaryOpKind::Plus);
return result;
mlir::Value VisitUnaryPlus(const UnaryOperator *e) {
return emitUnaryPlusOrMinus(e, cir::UnaryOpKind::Plus);
}

mlir::Value VisitUnaryMinus(const UnaryOperator *e,
QualType promotionType = QualType()) {
if (!promotionType.isNull())
cgf.cgm.errorNYI(e->getSourceRange(), "VisitUnaryMinus: promotionType");
assert(!cir::MissingFeatures::opUnaryPromotionType());
mlir::Value result = emitUnaryPlusOrMinus(e, cir::UnaryOpKind::Minus);
return result;
mlir::Value VisitUnaryMinus(const UnaryOperator *e) {
return emitUnaryPlusOrMinus(e, cir::UnaryOpKind::Minus);
}

mlir::Value emitUnaryPlusOrMinus(const UnaryOperator *e,
cir::UnaryOpKind kind) {
ignoreResultAssign = false;

assert(!cir::MissingFeatures::opUnaryPromotionType());
mlir::Value operand = Visit(e->getSubExpr());
QualType promotionType = getPromotionType(e->getSubExpr()->getType());

mlir::Value operand;
if (!promotionType.isNull())
operand = cgf.emitPromotedScalarExpr(e->getSubExpr(), promotionType);
else
operand = Visit(e->getSubExpr());

bool nsw =
kind == cir::UnaryOpKind::Minus && e->getType()->isSignedIntegerType();

// NOTE: LLVM codegen will lower this directly to either a FNeg
// or a Sub instruction. In CIR this will be handled later in LowerToLLVM.
return emitUnaryOp(e, kind, operand, nsw);
mlir::Value result = emitUnaryOp(e, kind, operand, nsw);
if (result && !promotionType.isNull())
return emitUnPromotedValue(result, e->getType());
return result;
}

mlir::Value emitUnaryOp(const UnaryOperator *e, cir::UnaryOpKind kind,
Expand Down
42 changes: 42 additions & 0 deletions clang/test/CIR/CodeGen/unary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,3 +424,45 @@ void chars(char c) {
c++; // CHECK: cir.unary(inc, %{{.+}}) : !s8i, !s8i
c--; // CHECK: cir.unary(dec, %{{.+}}) : !s8i, !s8i
}

_Float16 fp16UPlus(_Float16 f) {
return +f;
}

// CHECK: cir.func @fp16UPlus({{.*}}) -> !cir.f16
// CHECK: %[[INPUT:.*]] = cir.load %[[F:.*]]
// CHECK: %[[PROMOTED:.*]] = cir.cast(floating, %[[INPUT]] : !cir.f16), !cir.float
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm having a touch of trouble understanding this, so can you clarify:

We're casting the f16 to float (f32) because we expect the hardware doesn't support f16 operations, right? So we cast it, do the math, then bring it back.

I find myself wondering why CIR doesn't need/shouldn't model this instead, and this shouldn't happen in CIR->LLVM-IR lowering?

Copy link
Member

@bcardosolopes bcardosolopes Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find myself wondering why CIR doesn't need/shouldn't model this instead, and this shouldn't happen in CIR->LLVM-IR lowering?

Yep, it does make sense, we do similar with other operations. If we go about this I suggest we land the direct version first and introduce a more higher level CIR in a follow up (and add that to incu too).

Some background: so far the approach has been not to raise too early unless we (a) have use case for those new ops, or (b) we feel it's a natural improvement and have time to work on it. Otherwise we start very similar to LLVM (as the safe known path) until an opportunity to raise appears. Depending on how hard it might be (e.g. when dealing with vtables, itanium logic, etc) we usually ask folks to do the most similar to LLVM first and in a subsquent PR work on a more higher level approach, this allows us to get functionality before potential design problems (and PRs not moving along because too many design iterations).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erichkeane This is an excellent question, and I'm glad you raised it this early in the process. As I've been working on upstreaming these operations, I found myself somewhat confused by the difference between type promotion that is happening because the C/C++ semantics require it (such as short-to-int promotion during increment/decrement) and promotion that's happening because the compile options want us to perform calculations with excess precision (such as the fp16-to-float promotion seen here). There's also going to be some type promotion associated with complex division, which is yet a third thing. The terminology in the code doesn't really distinguish between these.

I believe the C/C++ standard promotions are represented directly in the AST, and so we don't need any special handling for them. I think you're right that we probably do not want to encode the other type promotions directly into CIR in the long run, but I'm not sure how the lowering would determine if this sort of promotion needs to happen. And if we lower to other intermediate dialects, do those dialects need to know about the promotion? I think if they don't, it's not going to happen, but it also seems like they might want to know what the real data type in the source was, especially if they're going to be making decisions about vectorizing or offloading to different hardware.

If we go about this I suggest we land the direct version first and introduce a more higher level CIR in a follow up (and add that to incu too).

@bcardosolopes When you say, "land the direct version first," do you mean proceed with this PR as is? That seems reasonable to me. I just wanted to make sure we're all in agreement.

Long term, I think we have at least two options: (1) generate CIR that operates directly on the FP16/BF16 types with no indication that promotion might be required and insert the promotion during lowering if it is needed, or (2) generate CIR that explicitly represents the fact that we're promoting the type to perform calculations with excess precision. It sounds like maybe you want to consider the second approach? Or am I misunderstanding?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the C/C++ standard promotions are represented directly in the AST, and so we don't need any special handling for them.

yes, that is correct, any language required conversions are done as a part of AST creation via casts.

When you say, "land the direct version first," do you mean proceed with this PR as is? That seems reasonable to me. I just wanted to make sure we're all in agreement.

That is my take from what he said. My summary of Bruno's post is: "When designing CIR we stayed as low-level to LLVM-IR as possible, except when we saw explicit benefit of making CIR a 'higher level' language. So merge it as-is now, and if we see benefit to figuring this out later, do it then".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say, "land the direct version first," do you mean proceed with this PR as is? That seems reasonable to me. I just wanted to make sure we're all in agreement.

Yes!

Copy link
Member

@bcardosolopes bcardosolopes Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long term, I think we have at least two options

Both (1) and (2) have different design tradeoffs and my experience has been that it varies depending on the use case in question. For this specific case, I'd say that if the promotions aren't crucial for analysis on top of CIR (right now), I'd defer the details to LLVM, staying with (1) - note that here the operation itself wraps the semantic, because it contains enough info for you to lower things properly later on. In case you can see the explicit promotions being helpful for other things (random speculative example: callconv lowering) then it might be worth doing (2). Either way, implementation experience will tell if you should move from one to the other approach over time - in CIR so far we've come back n forth with similar design choices as part of evolving things over time.

// CHECK: %[[RESULT:.*]] = cir.unary(plus, %[[PROMOTED]])
// CHECK: %[[UNPROMOTED:.*]] = cir.cast(floating, %[[RESULT]] : !cir.float), !cir.f16

// LLVM: define half @fp16UPlus({{.*}})
// LLVM: %[[F_LOAD:.*]] = load half, ptr %{{.*}}, align 2
// LLVM: %[[PROMOTED:.*]] = fpext half %[[F_LOAD]] to float
// LLVM: %[[UNPROMOTED:.*]] = fptrunc float %[[PROMOTED]] to half

// OGCG: define{{.*}} half @_Z9fp16UPlusDF16_({{.*}})
// OGCG: %[[F_LOAD:.*]] = load half, ptr %{{.*}}, align 2
// OGCG: %[[PROMOTED:.*]] = fpext half %[[F_LOAD]] to float
// OGCG: %[[UNPROMOTED:.*]] = fptrunc float %[[PROMOTED]] to half

_Float16 fp16UMinus(_Float16 f) {
return -f;
}

// CHECK: cir.func @fp16UMinus({{.*}}) -> !cir.f16
// CHECK: %[[INPUT:.*]] = cir.load %[[F:.*]]
// CHECK: %[[PROMOTED:.*]] = cir.cast(floating, %[[INPUT]] : !cir.f16), !cir.float
// CHECK: %[[RESULT:.*]] = cir.unary(minus, %[[PROMOTED]])
// CHECK: %[[UNPROMOTED:.*]] = cir.cast(floating, %[[RESULT]] : !cir.float), !cir.f16

// LLVM: define half @fp16UMinus({{.*}})
// LLVM: %[[F_LOAD:.*]] = load half, ptr %{{.*}}, align 2
// LLVM: %[[PROMOTED:.*]] = fpext half %[[F_LOAD]] to float
// LLVM: %[[RESULT:.*]] = fneg float %[[PROMOTED]]
// LLVM: %[[UNPROMOTED:.*]] = fptrunc float %[[RESULT]] to half

// OGCG: define{{.*}} half @_Z10fp16UMinusDF16_({{.*}})
// OGCG: %[[F_LOAD:.*]] = load half, ptr %{{.*}}, align 2
// OGCG: %[[PROMOTED:.*]] = fpext half %[[F_LOAD]] to float
// OGCG: %[[RESULT:.*]] = fneg float %[[PROMOTED]]
// OGCG: %[[UNPROMOTED:.*]] = fptrunc float %[[RESULT]] to half