[cir-translate] Fix crash issue where the data layout string is missing

[darkbuck]

• Targets like 'aarch64' or 'arm' only populate the data layout string after the constructor. Need to call 'CreateTargetInfo' to setup them properly.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: None (darkbuck)

Changes

• Targets like 'aarch64' or 'arm' only populate the data layout string after the constructor. Need to call 'CreateTargetInfo' to setup them properly.

---

Full diff: https://github.com/llvm/llvm-project/pull/147209.diff

2 Files Affected:

• (modified) clang/test/CIR/Lowering/select.cir (+2-2)
• (modified) clang/tools/cir-translate/cir-translate.cpp (+10-2)

diff --git a/clang/test/CIR/Lowering/select.cir b/clang/test/CIR/Lowering/select.cir
index 71ca79a390e8b..37f9789e6b0f6 100644
--- a/clang/test/CIR/Lowering/select.cir
+++ b/clang/test/CIR/Lowering/select.cir
@@ -1,5 +1,5 @@
-// RUN: cir-translate -cir-to-llvmir --disable-cc-lowering -o %t.ll %s
-// RUN: FileCheck --input-file=%t.ll -check-prefix=LLVM %s
+// RUN: cir-translate -cir-to-llvmir --disable-cc-lowering -o - %s | FileCheck -check-prefix=LLVM %s
+// RUN: cir-translate -target aarch64 -cir-to-llvmir --disable-cc-lowering -o - %s | FileCheck -check-prefix=LLVM %s
 
 !s32i = !cir.int<s, 32>
 
diff --git a/clang/tools/cir-translate/cir-translate.cpp b/clang/tools/cir-translate/cir-translate.cpp
index e0e9414602a16..06bb31f677880 100644
--- a/clang/tools/cir-translate/cir-translate.cpp
+++ b/clang/tools/cir-translate/cir-translate.cpp
@@ -25,6 +25,9 @@
 #include "llvm/IR/Module.h"
 #include "llvm/TargetParser/Host.h"
 
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
 #include "clang/CIR/Dialect/Passes.h"
@@ -82,12 +85,17 @@ llvm::LogicalResult prepareCIRModuleDataLayout(mlir::ModuleOp mod,
 
   // Data layout is fully determined by the target triple. Here we only pass the
   // triple to get the data layout.
+  llvm::IntrusiveRefCntPtr<clang::DiagnosticIDs> diagID(
+      new clang::DiagnosticIDs);
+  clang::DiagnosticOptions diagOpts;
+  clang::DiagnosticsEngine diagnostics(diagID, diagOpts,
+                                       new clang::IgnoringDiagConsumer());
   llvm::Triple triple(rawTriple);
   clang::TargetOptions targetOptions;
   targetOptions.Triple = rawTriple;
   // FIXME: AllocateTarget is a big deal. Better make it a global state.
-  std::unique_ptr<clang::TargetInfo> targetInfo =
-      clang::targets::AllocateTarget(llvm::Triple(rawTriple), targetOptions);
+  llvm::IntrusiveRefCntPtr<clang::TargetInfo> targetInfo =
+      clang::TargetInfo::CreateTargetInfo(diagnostics, targetOptions);
   if (!targetInfo) {
     mod.emitError() << "error: invalid target triple '" << rawTriple << "'\n";
     return llvm::failure();

[bcardosolopes]

I was hoping we had a CreateTargetInfo variation that could do the diagnostic boilerplate, but looks like we don't. Any specific reason we need to switch on using IntrusiveRefCntPtr here?

[andykaylor]

Maybe we could add one? It seems there are multiple other callers that similarly don't use these.

[bcardosolopes]

+1 for adding a variant

[darkbuck]

That's copied from other usage of CreateTargetInfo. It could be replaced with unique_ptr.

[darkbuck]

TargetInfo, as well as DiagnosticsID and DiagnosticsEngine, are all derived from RefCountBase, where IntrusiveRefCntPtr is used to manage their heap allocations. It'd be better to manage their heap allocations in the same consistent manner, even though it won't make a semantic difference from unique_ptr in the current cir-translate.

[bcardosolopes]

re IntrusiveRefCntPtr: yea, I see other examples around now.

[bcardosolopes]

I'm fine with this PR as is if you can put a follow up PR that tries adding the create* variant mentioned above!

[bcardosolopes]

LGTM pending my last comment