[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.

[andykaylor]

Why are you allocating a ref-counted DiagnosticIDs but using a local variable for DiagnosticOptions? The existing examples I found in the clang code seem to use ref-counted pointers for both.

[darkbuck]

Through examples, only DiagnosticIDs uses smart pointer. I will double-check.

[andykaylor]

Can you update this comment to just refer to TargetOptions? It seems that we're only using the data layout string here, but there are many other things in TargetOptions that might be relevant at some point.