Skip to content

[CIR] Realign CIR-to-LLVM IR lowering code with incubator #129293

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 3 commits into from
Feb 28, 2025
Merged

[CIR] Realign CIR-to-LLVM IR lowering code with incubator #129293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb41af6
[CIR] Replace CIRAttrVisitor with TypeSwitch
andykaylor Feb 28, 2025
cba1518
Re-align lowering code with incubator implementation
andykaylor Feb 28, 2025
c76fb37
Restore use of GlobalInitAttrRewriter
andykaylor Feb 28, 2025
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
52 changes: 0 additions & 52 deletions clang/include/clang/CIR/Dialect/IR/CIRAttrVisitor.h
View file
Open in desktop

This file was deleted.

122 changes: 69 additions & 53 deletions clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
#include "mlir/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.h"
#include "mlir/Target/LLVMIR/Export.h"
#include "mlir/Transforms/DialectConversion.h"
#include "clang/CIR/Dialect/IR/CIRAttrVisitor.h"
#include "clang/CIR/Dialect/IR/CIRDialect.h"
#include "clang/CIR/MissingFeatures.h"
#include "clang/CIR/Passes.h"
#include "llvm/ADT/TypeSwitch.h"
#include "llvm/IR/Module.h"
#include "llvm/Support/TimeProfiler.h"

Expand All @@ -37,63 +37,78 @@ using namespace llvm;
namespace cir {
namespace direct {

class CIRAttrToValue : public CirAttrVisitor<CIRAttrToValue, mlir::Value> {
class CIRAttrToValue {
public:
CIRAttrToValue(mlir::Operation *parentOp,
mlir::ConversionPatternRewriter &rewriter,
const mlir::TypeConverter *converter)
: parentOp(parentOp), rewriter(rewriter), converter(converter) {}

mlir::Value lowerCirAttrAsValue(mlir::Attribute attr) { return visit(attr); }

mlir::Value visitCirIntAttr(cir::IntAttr intAttr) {
mlir::Location loc = parentOp->getLoc();
return rewriter.create<mlir::LLVM::ConstantOp>(
loc, converter->convertType(intAttr.getType()), intAttr.getValue());
}

mlir::Value visitCirFPAttr(cir::FPAttr fltAttr) {
mlir::Location loc = parentOp->getLoc();
return rewriter.create<mlir::LLVM::ConstantOp>(
loc, converter->convertType(fltAttr.getType()), fltAttr.getValue());
mlir::Value visit(mlir::Attribute attr) {
return llvm::TypeSwitch<mlir::Attribute, mlir::Value>(attr)
.Case<cir::IntAttr, cir::FPAttr, cir::ConstPtrAttr>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its a shame this is such a manual list/sub-par interface here :/ it would be way neater if it was able to deduce these. sigh

That said, I think this is quite a bit of a nicer interface, so I'm ok with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it needs to be a manual list because it's never going to be the complete list. These are just the types we intend to handle here, with other types going to the Default() handler.

[&](auto attrT) { return visitCirAttr(attrT); })
.Default([&](auto attrT) { return mlir::Value(); });
}

mlir::Value visitCirConstPtrAttr(cir::ConstPtrAttr ptrAttr) {
mlir::Location loc = parentOp->getLoc();
if (ptrAttr.isNullValue()) {
return rewriter.create<mlir::LLVM::ZeroOp>(
loc, converter->convertType(ptrAttr.getType()));
}
mlir::DataLayout layout(parentOp->getParentOfType<mlir::ModuleOp>());
mlir::Value ptrVal = rewriter.create<mlir::LLVM::ConstantOp>(
loc,
rewriter.getIntegerType(layout.getTypeSizeInBits(ptrAttr.getType())),
ptrAttr.getValue().getInt());
return rewriter.create<mlir::LLVM::IntToPtrOp>(
loc, converter->convertType(ptrAttr.getType()), ptrVal);
}
mlir::Value visitCirAttr(cir::IntAttr intAttr);
mlir::Value visitCirAttr(cir::FPAttr fltAttr);
mlir::Value visitCirAttr(cir::ConstPtrAttr ptrAttr);

private:
mlir::Operation *parentOp;
mlir::ConversionPatternRewriter &rewriter;
const mlir::TypeConverter *converter;
};

/// IntAttr visitor.
mlir::Value CIRAttrToValue::visitCirAttr(cir::IntAttr intAttr) {
mlir::Location loc = parentOp->getLoc();
return rewriter.create<mlir::LLVM::ConstantOp>(
loc, converter->convertType(intAttr.getType()), intAttr.getValue());
}

/// ConstPtrAttr visitor.
mlir::Value CIRAttrToValue::visitCirAttr(cir::ConstPtrAttr ptrAttr) {
mlir::Location loc = parentOp->getLoc();
if (ptrAttr.isNullValue()) {
return rewriter.create<mlir::LLVM::ZeroOp>(
loc, converter->convertType(ptrAttr.getType()));
}
mlir::DataLayout layout(parentOp->getParentOfType<mlir::ModuleOp>());
mlir::Value ptrVal = rewriter.create<mlir::LLVM::ConstantOp>(
loc, rewriter.getIntegerType(layout.getTypeSizeInBits(ptrAttr.getType())),
ptrAttr.getValue().getInt());
return rewriter.create<mlir::LLVM::IntToPtrOp>(
loc, converter->convertType(ptrAttr.getType()), ptrVal);
}

/// FPAttr visitor.
mlir::Value CIRAttrToValue::visitCirAttr(cir::FPAttr fltAttr) {
mlir::Location loc = parentOp->getLoc();
return rewriter.create<mlir::LLVM::ConstantOp>(
loc, converter->convertType(fltAttr.getType()), fltAttr.getValue());
}

// This class handles rewriting initializer attributes for types that do not
// require region initialization.
class GlobalInitAttrRewriter
: public CirAttrVisitor<GlobalInitAttrRewriter, mlir::Attribute> {
class GlobalInitAttrRewriter {
public:
GlobalInitAttrRewriter(mlir::Type type,
mlir::ConversionPatternRewriter &rewriter)
: llvmType(type), rewriter(rewriter) {}

mlir::Attribute rewriteInitAttr(mlir::Attribute attr) { return visit(attr); }
mlir::Attribute visit(mlir::Attribute attr) {
return llvm::TypeSwitch<mlir::Attribute, mlir::Attribute>(attr)
.Case<cir::IntAttr, cir::FPAttr>(
[&](auto attrT) { return visitCirAttr(attrT); })
.Default([&](auto attrT) { return mlir::Attribute(); });
}

mlir::Attribute visitCirIntAttr(cir::IntAttr attr) {
mlir::Attribute visitCirAttr(cir::IntAttr attr) {
return rewriter.getIntegerAttr(llvmType, attr.getValue());
}
mlir::Attribute visitCirFPAttr(cir::FPAttr attr) {
mlir::Attribute visitCirAttr(cir::FPAttr attr) {
return rewriter.getFloatAttr(llvmType, attr.getValue());
}

Expand Down Expand Up @@ -124,12 +139,6 @@ struct ConvertCIRToLLVMPass
StringRef getArgument() const override { return "cir-flat-to-llvm"; }
};

bool CIRToLLVMGlobalOpLowering::attrRequiresRegionInitialization(
mlir::Attribute attr) const {
// There will be more cases added later.
return isa<cir::ConstPtrAttr>(attr);
}

/// Replace CIR global with a region initialized LLVM global and update
/// insertion point to the end of the initializer block.
void CIRToLLVMGlobalOpLowering::setupRegionInitializedLLVMGlobalOp(
Expand Down Expand Up @@ -176,8 +185,8 @@ CIRToLLVMGlobalOpLowering::matchAndRewriteRegionInitializedGlobal(
// to the appropriate value.
const mlir::Location loc = op.getLoc();
setupRegionInitializedLLVMGlobalOp(op, rewriter);
CIRAttrToValue attrVisitor(op, rewriter, typeConverter);
mlir::Value value = attrVisitor.lowerCirAttrAsValue(init);
CIRAttrToValue valueConverter(op, rewriter, typeConverter);
mlir::Value value = valueConverter.visit(init);
rewriter.create<mlir::LLVM::ReturnOp>(loc, value);
return mlir::success();
}
Expand All @@ -188,12 +197,6 @@ mlir::LogicalResult CIRToLLVMGlobalOpLowering::matchAndRewrite(

std::optional<mlir::Attribute> init = op.getInitialValue();

// If we have an initializer and it requires region initialization, handle
// that separately
if (init.has_value() && attrRequiresRegionInitialization(init.value())) {
return matchAndRewriteRegionInitializedGlobal(op, init.value(), rewriter);
}

// Fetch required values to create LLVM op.
const mlir::Type cirSymType = op.getSymType();

Expand All @@ -218,12 +221,25 @@ mlir::LogicalResult CIRToLLVMGlobalOpLowering::matchAndRewrite(
SmallVector<mlir::NamedAttribute> attributes;

if (init.has_value()) {
GlobalInitAttrRewriter initRewriter(llvmType, rewriter);
init = initRewriter.rewriteInitAttr(init.value());
// If initRewriter returned a null attribute, init will have a value but
// the value will be null. If that happens, initRewriter didn't handle the
// attribute type. It probably needs to be added to GlobalInitAttrRewriter.
if (!init.value()) {
if (mlir::isa<cir::FPAttr, cir::IntAttr>(init.value())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section is a bit worse unfortunately. We might consider extracting this type at one point.

Why did we remove the use of GlobalInitAttrRewriter (or am I missing a use elsewhere?), and do we intend to bring it back? Could it be used here instead like it was before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GlobalInitAttrRewriter would have used a TypeSwitch for its internal implementation, and since each type being handled/visited is just a single line I thought it was just as clean to put it inline here. I can move it back to using a separate class if you like.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am just asking for 1 or the other. IF we don't need GlobalInitAttrRewriter (It looks like we don't as you've removed its usage?) we should remove it, not update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I see that I did both -- I modified GlobalInitAttrRewriter to use TypeSwitch but I rewrote this code to not use it. I guess that was an incomplete refactoring, but the good new is that it will make it easy to update it as you've suggested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, cool :D I was beginning to think I was missing something, like another use of it! I don't mind which you do (remove it and leave inline, or switch to the type). There is a 'line' I suspect where 'leave inline' becomes too big/annoying and we want to move it, but we are far from it at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The incubator only has three types that get handled here, but it does look a bit cleaner with these being handled separately. There is a possibility that the if above could get out of sync with the types handled in GlobalInitAttrRewriter but that would trigger the error below, so we'd catch it in development.

GlobalInitAttrRewriter initRewriter(llvmType, rewriter);
init = initRewriter.visit(init.value());
// If initRewriter returned a null attribute, init will have a value but
// the value will be null. If that happens, initRewriter didn't handle the
// attribute type. It probably needs to be added to
// GlobalInitAttrRewriter.
if (!init.value()) {
op.emitError() << "unsupported initializer '" << init.value() << "'";
return mlir::failure();
}
} else if (mlir::isa<cir::ConstPtrAttr>(init.value())) {
// TODO(cir): once LLVM's dialect has proper equivalent attributes this
// should be updated. For now, we use a custom op to initialize globals
// to the appropriate value.
return matchAndRewriteRegionInitializedGlobal(op, init.value(), rewriter);
} else {
// We will only get here if new initializer types are added and this
// code is not updated to handle them.
op.emitError() << "unsupported initializer '" << init.value() << "'";
return mlir::failure();
}
Expand Down
2 changes: 0 additions & 2 deletions clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ class CIRToLLVMGlobalOpLowering
mlir::ConversionPatternRewriter &rewriter) const override;

private:
bool attrRequiresRegionInitialization(mlir::Attribute attr) const;

mlir::LogicalResult matchAndRewriteRegionInitializedGlobal(
cir::GlobalOp op, mlir::Attribute init,
mlir::ConversionPatternRewriter &rewriter) const;
Expand Down