[GlobalISel] use constexpr LLT types when creating ISel data#191574
Merged
Conversation
Member
|
@llvm/pr-subscribers-llvm-globalisel Author: Stanley Gambarin (stanleygambarin) ChangesThe GlobalISel uses a lookup table to map LLTs which is constructed Workaround is the use constexpr LLT, which encodes INTEGER/FLOAT LLT. Assisted-by: Claude Opus 4.6 Full diff: https://github.com/llvm/llvm-project/pull/191574.diff 3 Files Affected:
diff --git a/llvm/include/llvm/CodeGenTypes/LowLevelType.h b/llvm/include/llvm/CodeGenTypes/LowLevelType.h
index 920bd9ad9bf51..884249ae99bcd 100644
--- a/llvm/include/llvm/CodeGenTypes/LowLevelType.h
+++ b/llvm/include/llvm/CodeGenTypes/LowLevelType.h
@@ -96,6 +96,14 @@ class LLT {
return LLT{Kind::INTEGER, ElementCount::getFixed(0), SizeInBits};
}
+ /// Constexpr integer type constructor that always produces the INTEGER kind,
+ /// regardless of the ExtendedLLT flag. Used by generated code (e.g.
+ /// TypeObjects arrays in GlobalISel) that must have stable types at static
+ /// initialization time.
+ static constexpr LLT integerExact(unsigned SizeInBits) {
+ return LLT{Kind::INTEGER, ElementCount::getFixed(0), SizeInBits};
+ }
+
static LLT floatingPoint(const FpSemantics &Sem) {
if (!getUseExtended())
return LLT::scalar(
diff --git a/llvm/unittests/CodeGen/LowLevelTypeTest.cpp b/llvm/unittests/CodeGen/LowLevelTypeTest.cpp
index 411d834647d70..d8a170c58cdca 100644
--- a/llvm/unittests/CodeGen/LowLevelTypeTest.cpp
+++ b/llvm/unittests/CodeGen/LowLevelTypeTest.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/CodeGen/LowLevelTypeUtils.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/LLVMContext.h"
@@ -449,4 +450,113 @@ TEST(LowLevelTypeTest, IsScalableVector) {
EXPECT_TRUE(LLT::scalable_vector(2, 32).isScalableVector());
EXPECT_TRUE(LLT::scalable_vector(1, 32).isScalableVector());
}
+
+// Demonstrate that LLT::integer() and LLT::floatIEEE() produce different types
+// depending on the ExtendedLLT flag, which causes DenseMap lookup failures when
+// a TypeObjects array is statically initialized before setUseExtended(true).
+// This is the bug fixed by integerExact() and the float-specific constructors
+// in the GlobalISel TableGen emitter.
+TEST(LowLevelTypeTest, ExtendedLLTDenseMapConsistency) {
+ // Save and restore the ExtendedLLT state.
+ bool OldExtended = LLT::getUseExtended();
+
+ // --- Phase 1: simulate static initialization (ExtendedLLT = false) ---
+ LLT::setUseExtended(false);
+
+ // These mimic what the *old* generated TypeObjects array contained:
+ // LLT::integer(N) falls back to LLT::scalar(N) when extended is off.
+ LLT OldI16 = LLT::integer(16);
+ LLT OldI32 = LLT::integer(32);
+ LLT OldF32 = LLT::floatIEEE(32);
+ LLT OldF64 = LLT::floatIEEE(64);
+
+ // All of these are just LLT::scalar(N) — no integer/float distinction.
+ EXPECT_TRUE(OldI16.isAnyScalar());
+ EXPECT_FALSE(OldI16.isInteger());
+ EXPECT_TRUE(OldI32.isAnyScalar());
+ EXPECT_FALSE(OldF32.isFloat());
+
+ // Build a DenseMap as ExecInfoTy does: TypeObjects[i] -> i
+ SmallDenseMap<LLT, unsigned, 64> TypeIDMap;
+ TypeIDMap[OldI16] = 2; // GILLT_i16
+ TypeIDMap[OldI32] = 3; // GILLT_i32
+ TypeIDMap[OldF32] = 8; // GILLT_f32
+ TypeIDMap[OldF64] = 9; // GILLT_f64
+
+ // --- Phase 2: simulate runtime (ExtendedLLT = true) ---
+ LLT::setUseExtended(true);
+
+ // At runtime, getLLTForType / MRI.getType() produces extended types.
+ LLT RuntimeI32 = LLT::integer(32);
+ LLT RuntimeF32 = LLT::floatIEEE(32);
+
+ // These are properly typed now.
+ EXPECT_TRUE(RuntimeI32.isInteger());
+ EXPECT_TRUE(RuntimeF32.isFloat());
+
+ // operator== considers scalar(32) and integer(32) equal (via the
+ // isAnyScalar() wildcard path), but their raw data differs:
+ EXPECT_EQ(OldI32, RuntimeI32);
+ EXPECT_NE(OldI32.getUniqueRAWLLTData(), RuntimeI32.getUniqueRAWLLTData());
+
+ // Similarly for float types:
+ EXPECT_EQ(OldF32, RuntimeF32);
+ EXPECT_NE(OldF32.getUniqueRAWLLTData(), RuntimeF32.getUniqueRAWLLTData());
+
+ // BUG: DenseMap hashes are based on getUniqueRAWLLTData(), so
+ // hash(scalar(32)) != hash(integer(32)) even though they're operator==.
+ // This violates the DenseMap contract and causes unreliable lookups.
+ EXPECT_NE(DenseMapInfo<LLT>::getHashValue(OldI32),
+ DenseMapInfo<LLT>::getHashValue(RuntimeI32));
+ EXPECT_NE(DenseMapInfo<LLT>::getHashValue(OldF32),
+ DenseMapInfo<LLT>::getHashValue(RuntimeF32));
+
+ // --- Phase 3: show that integerExact / float32 fix the problem ---
+ SmallDenseMap<LLT, unsigned, 64> FixedTypeIDMap;
+
+ // New generated code uses integerExact() and float32() etc., which are
+ // constexpr and always produce the correct Kind regardless of ExtendedLLT.
+ FixedTypeIDMap[LLT::integerExact(16)] = 2;
+ FixedTypeIDMap[LLT::integerExact(32)] = 3;
+ FixedTypeIDMap[LLT::float32()] = 8;
+ FixedTypeIDMap[LLT::float64()] = 9;
+ FixedTypeIDMap[LLT::fixed_vector(4, LLT::integerExact(16))] = 18;
+
+ LLT RuntimeI16 = LLT::integer(16);
+ LLT RuntimeV4I16 = LLT::fixed_vector(4, RuntimeI16);
+ LLT RuntimeF64 = LLT::floatIEEE(64);
+
+ // The raw data of integerExact(N) matches integer(N) when extended is on:
+ EXPECT_EQ(LLT::integerExact(32).getUniqueRAWLLTData(),
+ RuntimeI32.getUniqueRAWLLTData());
+ EXPECT_EQ(LLT::float32().getUniqueRAWLLTData(),
+ RuntimeF32.getUniqueRAWLLTData());
+
+ // So DenseMap hashes also match:
+ EXPECT_EQ(DenseMapInfo<LLT>::getHashValue(LLT::integerExact(32)),
+ DenseMapInfo<LLT>::getHashValue(RuntimeI32));
+ EXPECT_EQ(DenseMapInfo<LLT>::getHashValue(LLT::float32()),
+ DenseMapInfo<LLT>::getHashValue(RuntimeF32));
+
+ // Runtime lookups now SUCCEED:
+ auto Fix_I32 = FixedTypeIDMap.find(RuntimeI32);
+ auto Fix_F32 = FixedTypeIDMap.find(RuntimeF32);
+ auto Fix_F64 = FixedTypeIDMap.find(RuntimeF64);
+ auto Fix_V4I16 = FixedTypeIDMap.find(RuntimeV4I16);
+
+ ASSERT_NE(Fix_I32, FixedTypeIDMap.end());
+ EXPECT_EQ(Fix_I32->second, 3u);
+
+ ASSERT_NE(Fix_F32, FixedTypeIDMap.end());
+ EXPECT_EQ(Fix_F32->second, 8u);
+
+ ASSERT_NE(Fix_F64, FixedTypeIDMap.end());
+ EXPECT_EQ(Fix_F64->second, 9u);
+
+ ASSERT_NE(Fix_V4I16, FixedTypeIDMap.end());
+ EXPECT_EQ(Fix_V4I16->second, 18u);
+
+ LLT::setUseExtended(OldExtended);
+}
+
}
diff --git a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
index 1968097f91983..48e1db1077c28 100644
--- a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
+++ b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
@@ -408,10 +408,36 @@ void LLTCodeGen::emitCxxEnumValue(raw_ostream &OS) const {
llvm_unreachable("Unhandled LLT");
}
+/// Emit a constexpr LLT constructor call for an IEEE floating-point type.
+/// Using specific constructors (float16, float32, etc.) instead of the generic
+/// floatIEEE() avoids static initialization order issues: floatIEEE() calls
+/// getUseExtended() which may return false during static init, producing
+/// LLT::scalar() instead of the intended float type.
+static void emitFloatIEEECxxConstructorCall(raw_ostream &OS,
+ unsigned SizeInBits) {
+ switch (SizeInBits) {
+ case 16:
+ OS << "LLT::float16()";
+ break;
+ case 32:
+ OS << "LLT::float32()";
+ break;
+ case 64:
+ OS << "LLT::float64()";
+ break;
+ case 128:
+ OS << "LLT::float128()";
+ break;
+ default:
+ OS << "LLT::floatIEEE(" << SizeInBits << ")";
+ break;
+ }
+}
+
void LLTCodeGen::emitCxxConstructorCall(raw_ostream &OS) const {
if (Ty.isScalar()) {
if (Ty.isInteger())
- OS << "LLT::integer(" << Ty.getScalarSizeInBits() << ")";
+ OS << "LLT::integerExact(" << Ty.getScalarSizeInBits() << ")";
else if (Ty.isBFloat16())
OS << "LLT::bfloat16()";
else if (Ty.isPPCF128())
@@ -419,7 +445,7 @@ void LLTCodeGen::emitCxxConstructorCall(raw_ostream &OS) const {
else if (Ty.isX86FP80())
OS << "LLT::x86fp80()";
else if (Ty.isFloat())
- OS << "LLT::floatIEEE(" << Ty.getScalarSizeInBits() << ")";
+ emitFloatIEEECxxConstructorCall(OS, Ty.getScalarSizeInBits());
else
OS << "LLT::scalar(" << Ty.getScalarSizeInBits() << ")";
return;
@@ -433,7 +459,7 @@ void LLTCodeGen::emitCxxConstructorCall(raw_ostream &OS) const {
LLT ElemTy = Ty.getElementType();
if (ElemTy.isInteger())
- OS << "LLT::integer(" << ElemTy.getScalarSizeInBits() << ")";
+ OS << "LLT::integerExact(" << ElemTy.getScalarSizeInBits() << ")";
else if (ElemTy.isBFloat16())
OS << "LLT::bfloat16()";
else if (ElemTy.isPPCF128())
@@ -441,7 +467,7 @@ void LLTCodeGen::emitCxxConstructorCall(raw_ostream &OS) const {
else if (ElemTy.isX86FP80())
OS << "LLT::x86fp80()";
else if (ElemTy.isFloat())
- OS << "LLT::floatIEEE(" << ElemTy.getScalarSizeInBits() << ")";
+ emitFloatIEEECxxConstructorCall(OS, ElemTy.getScalarSizeInBits());
else
OS << "LLT::scalar(" << Ty.getScalarSizeInBits() << ")";
OS << ")";
|
Member
|
@llvm/pr-subscribers-tablegen Author: Stanley Gambarin (stanleygambarin) ChangesThe GlobalISel uses a lookup table to map LLTs which is constructed Workaround is the use constexpr LLT, which encodes INTEGER/FLOAT LLT. Assisted-by: Claude Opus 4.6 Full diff: https://github.com/llvm/llvm-project/pull/191574.diff 3 Files Affected:
diff --git a/llvm/include/llvm/CodeGenTypes/LowLevelType.h b/llvm/include/llvm/CodeGenTypes/LowLevelType.h
index 920bd9ad9bf51..884249ae99bcd 100644
--- a/llvm/include/llvm/CodeGenTypes/LowLevelType.h
+++ b/llvm/include/llvm/CodeGenTypes/LowLevelType.h
@@ -96,6 +96,14 @@ class LLT {
return LLT{Kind::INTEGER, ElementCount::getFixed(0), SizeInBits};
}
+ /// Constexpr integer type constructor that always produces the INTEGER kind,
+ /// regardless of the ExtendedLLT flag. Used by generated code (e.g.
+ /// TypeObjects arrays in GlobalISel) that must have stable types at static
+ /// initialization time.
+ static constexpr LLT integerExact(unsigned SizeInBits) {
+ return LLT{Kind::INTEGER, ElementCount::getFixed(0), SizeInBits};
+ }
+
static LLT floatingPoint(const FpSemantics &Sem) {
if (!getUseExtended())
return LLT::scalar(
diff --git a/llvm/unittests/CodeGen/LowLevelTypeTest.cpp b/llvm/unittests/CodeGen/LowLevelTypeTest.cpp
index 411d834647d70..d8a170c58cdca 100644
--- a/llvm/unittests/CodeGen/LowLevelTypeTest.cpp
+++ b/llvm/unittests/CodeGen/LowLevelTypeTest.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/CodeGen/LowLevelTypeUtils.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/LLVMContext.h"
@@ -449,4 +450,113 @@ TEST(LowLevelTypeTest, IsScalableVector) {
EXPECT_TRUE(LLT::scalable_vector(2, 32).isScalableVector());
EXPECT_TRUE(LLT::scalable_vector(1, 32).isScalableVector());
}
+
+// Demonstrate that LLT::integer() and LLT::floatIEEE() produce different types
+// depending on the ExtendedLLT flag, which causes DenseMap lookup failures when
+// a TypeObjects array is statically initialized before setUseExtended(true).
+// This is the bug fixed by integerExact() and the float-specific constructors
+// in the GlobalISel TableGen emitter.
+TEST(LowLevelTypeTest, ExtendedLLTDenseMapConsistency) {
+ // Save and restore the ExtendedLLT state.
+ bool OldExtended = LLT::getUseExtended();
+
+ // --- Phase 1: simulate static initialization (ExtendedLLT = false) ---
+ LLT::setUseExtended(false);
+
+ // These mimic what the *old* generated TypeObjects array contained:
+ // LLT::integer(N) falls back to LLT::scalar(N) when extended is off.
+ LLT OldI16 = LLT::integer(16);
+ LLT OldI32 = LLT::integer(32);
+ LLT OldF32 = LLT::floatIEEE(32);
+ LLT OldF64 = LLT::floatIEEE(64);
+
+ // All of these are just LLT::scalar(N) — no integer/float distinction.
+ EXPECT_TRUE(OldI16.isAnyScalar());
+ EXPECT_FALSE(OldI16.isInteger());
+ EXPECT_TRUE(OldI32.isAnyScalar());
+ EXPECT_FALSE(OldF32.isFloat());
+
+ // Build a DenseMap as ExecInfoTy does: TypeObjects[i] -> i
+ SmallDenseMap<LLT, unsigned, 64> TypeIDMap;
+ TypeIDMap[OldI16] = 2; // GILLT_i16
+ TypeIDMap[OldI32] = 3; // GILLT_i32
+ TypeIDMap[OldF32] = 8; // GILLT_f32
+ TypeIDMap[OldF64] = 9; // GILLT_f64
+
+ // --- Phase 2: simulate runtime (ExtendedLLT = true) ---
+ LLT::setUseExtended(true);
+
+ // At runtime, getLLTForType / MRI.getType() produces extended types.
+ LLT RuntimeI32 = LLT::integer(32);
+ LLT RuntimeF32 = LLT::floatIEEE(32);
+
+ // These are properly typed now.
+ EXPECT_TRUE(RuntimeI32.isInteger());
+ EXPECT_TRUE(RuntimeF32.isFloat());
+
+ // operator== considers scalar(32) and integer(32) equal (via the
+ // isAnyScalar() wildcard path), but their raw data differs:
+ EXPECT_EQ(OldI32, RuntimeI32);
+ EXPECT_NE(OldI32.getUniqueRAWLLTData(), RuntimeI32.getUniqueRAWLLTData());
+
+ // Similarly for float types:
+ EXPECT_EQ(OldF32, RuntimeF32);
+ EXPECT_NE(OldF32.getUniqueRAWLLTData(), RuntimeF32.getUniqueRAWLLTData());
+
+ // BUG: DenseMap hashes are based on getUniqueRAWLLTData(), so
+ // hash(scalar(32)) != hash(integer(32)) even though they're operator==.
+ // This violates the DenseMap contract and causes unreliable lookups.
+ EXPECT_NE(DenseMapInfo<LLT>::getHashValue(OldI32),
+ DenseMapInfo<LLT>::getHashValue(RuntimeI32));
+ EXPECT_NE(DenseMapInfo<LLT>::getHashValue(OldF32),
+ DenseMapInfo<LLT>::getHashValue(RuntimeF32));
+
+ // --- Phase 3: show that integerExact / float32 fix the problem ---
+ SmallDenseMap<LLT, unsigned, 64> FixedTypeIDMap;
+
+ // New generated code uses integerExact() and float32() etc., which are
+ // constexpr and always produce the correct Kind regardless of ExtendedLLT.
+ FixedTypeIDMap[LLT::integerExact(16)] = 2;
+ FixedTypeIDMap[LLT::integerExact(32)] = 3;
+ FixedTypeIDMap[LLT::float32()] = 8;
+ FixedTypeIDMap[LLT::float64()] = 9;
+ FixedTypeIDMap[LLT::fixed_vector(4, LLT::integerExact(16))] = 18;
+
+ LLT RuntimeI16 = LLT::integer(16);
+ LLT RuntimeV4I16 = LLT::fixed_vector(4, RuntimeI16);
+ LLT RuntimeF64 = LLT::floatIEEE(64);
+
+ // The raw data of integerExact(N) matches integer(N) when extended is on:
+ EXPECT_EQ(LLT::integerExact(32).getUniqueRAWLLTData(),
+ RuntimeI32.getUniqueRAWLLTData());
+ EXPECT_EQ(LLT::float32().getUniqueRAWLLTData(),
+ RuntimeF32.getUniqueRAWLLTData());
+
+ // So DenseMap hashes also match:
+ EXPECT_EQ(DenseMapInfo<LLT>::getHashValue(LLT::integerExact(32)),
+ DenseMapInfo<LLT>::getHashValue(RuntimeI32));
+ EXPECT_EQ(DenseMapInfo<LLT>::getHashValue(LLT::float32()),
+ DenseMapInfo<LLT>::getHashValue(RuntimeF32));
+
+ // Runtime lookups now SUCCEED:
+ auto Fix_I32 = FixedTypeIDMap.find(RuntimeI32);
+ auto Fix_F32 = FixedTypeIDMap.find(RuntimeF32);
+ auto Fix_F64 = FixedTypeIDMap.find(RuntimeF64);
+ auto Fix_V4I16 = FixedTypeIDMap.find(RuntimeV4I16);
+
+ ASSERT_NE(Fix_I32, FixedTypeIDMap.end());
+ EXPECT_EQ(Fix_I32->second, 3u);
+
+ ASSERT_NE(Fix_F32, FixedTypeIDMap.end());
+ EXPECT_EQ(Fix_F32->second, 8u);
+
+ ASSERT_NE(Fix_F64, FixedTypeIDMap.end());
+ EXPECT_EQ(Fix_F64->second, 9u);
+
+ ASSERT_NE(Fix_V4I16, FixedTypeIDMap.end());
+ EXPECT_EQ(Fix_V4I16->second, 18u);
+
+ LLT::setUseExtended(OldExtended);
+}
+
}
diff --git a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
index 1968097f91983..48e1db1077c28 100644
--- a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
+++ b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
@@ -408,10 +408,36 @@ void LLTCodeGen::emitCxxEnumValue(raw_ostream &OS) const {
llvm_unreachable("Unhandled LLT");
}
+/// Emit a constexpr LLT constructor call for an IEEE floating-point type.
+/// Using specific constructors (float16, float32, etc.) instead of the generic
+/// floatIEEE() avoids static initialization order issues: floatIEEE() calls
+/// getUseExtended() which may return false during static init, producing
+/// LLT::scalar() instead of the intended float type.
+static void emitFloatIEEECxxConstructorCall(raw_ostream &OS,
+ unsigned SizeInBits) {
+ switch (SizeInBits) {
+ case 16:
+ OS << "LLT::float16()";
+ break;
+ case 32:
+ OS << "LLT::float32()";
+ break;
+ case 64:
+ OS << "LLT::float64()";
+ break;
+ case 128:
+ OS << "LLT::float128()";
+ break;
+ default:
+ OS << "LLT::floatIEEE(" << SizeInBits << ")";
+ break;
+ }
+}
+
void LLTCodeGen::emitCxxConstructorCall(raw_ostream &OS) const {
if (Ty.isScalar()) {
if (Ty.isInteger())
- OS << "LLT::integer(" << Ty.getScalarSizeInBits() << ")";
+ OS << "LLT::integerExact(" << Ty.getScalarSizeInBits() << ")";
else if (Ty.isBFloat16())
OS << "LLT::bfloat16()";
else if (Ty.isPPCF128())
@@ -419,7 +445,7 @@ void LLTCodeGen::emitCxxConstructorCall(raw_ostream &OS) const {
else if (Ty.isX86FP80())
OS << "LLT::x86fp80()";
else if (Ty.isFloat())
- OS << "LLT::floatIEEE(" << Ty.getScalarSizeInBits() << ")";
+ emitFloatIEEECxxConstructorCall(OS, Ty.getScalarSizeInBits());
else
OS << "LLT::scalar(" << Ty.getScalarSizeInBits() << ")";
return;
@@ -433,7 +459,7 @@ void LLTCodeGen::emitCxxConstructorCall(raw_ostream &OS) const {
LLT ElemTy = Ty.getElementType();
if (ElemTy.isInteger())
- OS << "LLT::integer(" << ElemTy.getScalarSizeInBits() << ")";
+ OS << "LLT::integerExact(" << ElemTy.getScalarSizeInBits() << ")";
else if (ElemTy.isBFloat16())
OS << "LLT::bfloat16()";
else if (ElemTy.isPPCF128())
@@ -441,7 +467,7 @@ void LLTCodeGen::emitCxxConstructorCall(raw_ostream &OS) const {
else if (ElemTy.isX86FP80())
OS << "LLT::x86fp80()";
else if (ElemTy.isFloat())
- OS << "LLT::floatIEEE(" << ElemTy.getScalarSizeInBits() << ")";
+ emitFloatIEEECxxConstructorCall(OS, ElemTy.getScalarSizeInBits());
else
OS << "LLT::scalar(" << Ty.getScalarSizeInBits() << ")";
OS << ")";
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Contributor
Author
|
alternative solution would be to modify LLT::operator== bool operator==(const LLT &RHS) const { // Legacy behavior for non-extended mode. however, that would break every comparison between integer(N) and scalar(N) when extended LLT is enabled, requiring complete switch to extended LLT for the whole backend. |
michalpaszkowski
requested review from
MrSidims,
Pierre-vh,
QuantumSegfault,
aemerson,
lialan,
spavloff and
tgymnich
April 11, 2026 00:09
Member
|
@stanleygambarin Thanks for the patch! I am fine with either solution. I don't see any downsides to the workaround apart from the fact that it is a workaround and the original problem is still present. The workaround is somewhat self-contained and we could always go with the alternative later. Also Going with the alternative would solve the actual problem, but will force each backend with CC @DenisGZM @tgymnich I am also adding reviewers from the #155107 Please take a look at the unit test and let us know your thoughts/preferences. |
michalpaszkowski
self-requested a review
April 11, 2026 00:12
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
Contributor
|
Thank you! I believe this solves #190873 that I was running into. In regards to preference, I don't really have one. I just want it to work. I'm building GlobalISel for Wasm here upstream, and intend to migrate to But as with most things, I'm sure this compat was done for a highly debated reason, so undoing it so soon seems silly. |
davemgreen
reviewed
Apr 11, 2026
davemgreen
left a comment
davemgreen
left a comment
Contributor
There was a problem hiding this comment.
Hi - Remove integerExact and emitFloatIEEECxxConstructorCall - this can use the explicit constructors directly (like OS << "LLT(LLT::Kind::INTEGER, ElementCount::getFixed(0), " << Ty.getScalarSizeInBits() << ")";. We might need to change the float calls to generate scalar types eventually too, it is worth adding a FIXME to say that we should clean it up once UseExtended is removed.
You can just update llvm/test/TableGen/GlobalISelEmitter/GlobalISelEmitter.td for the test.
lialan
reviewed
Apr 12, 2026
| OS << "LLT::float128()"; | ||
| break; | ||
| default: | ||
| OS << "LLT::floatIEEE(" << SizeInBits << ")"; |
Member
There was a problem hiding this comment.
is this conflicting the function comment which says:
Using specific constructors (float16, float32, etc.) instead of the generic ... floatIEEE()
Contributor
|
Hi! I think using exact LLT types in generated files is the preferred solution. This is explicit and expected behavior, as the targets themselves enable it. At the same time targets have some space for use scalars and switch them to exact types. |
stanleygambarin
force-pushed
the
extended_llt_fix
branch
from
April 13, 2026 16:29
ff85791 to
65cf3a1
Compare
davemgreen
reviewed
Apr 14, 2026
davemgreen
left a comment
davemgreen
left a comment
Contributor
There was a problem hiding this comment.
I was thinking that all the types should be using the low level constructors, including float versions. At some point in the future, because we have targets that do not enable ExtendedLLT, the float32() and similar functions might need to generate scalar types too. We can fix this in the future if it comes up though.
| if (Ty.isScalar()) { | ||
| if (Ty.isInteger()) | ||
| OS << "LLT::integer(" << Ty.getScalarSizeInBits() << ")"; | ||
| OS << "LLT(LLT::Kind::INTEGER, ElementCount::getFixed(0), " |
Contributor
There was a problem hiding this comment.
Create a lambda that prints the LLT type and reuse it for vector elements too?
| EXPECT_TRUE(LLT::scalable_vector(1, 32).isScalableVector()); | ||
| } | ||
|
|
||
| // Demonstrate that LLT::integer() and LLT::floatIEEE() produce different types |
Contributor
There was a problem hiding this comment.
This test looks quite AI generated (quite verbose and not of a lot of value). Is it worth keeping or are the existing tests enough to show the issue is fixed? The real test is whether we can select instructions properly, and it hopefully won't be long until we have a real target working in tree.
davemgreen
approved these changes
Apr 14, 2026
davemgreen
left a comment
davemgreen
left a comment
Contributor
There was a problem hiding this comment.
Other than my comments this LGTM
The GlobalISel uses a lookup table to map LLTs which is constructed prior to initialization of extended LLT functionality, resulting in ANY_SCALAR entries. During instruction selection, a hash-based lookup is done on actual INTEGER/FLOAT LLTs. But hash values of ANY_SCALAR do not match those of INTEGER/FLOAT, causing a failure. Workaround is the use constexpr LLT, which encodes INTEGER/FLOAT LLT. Assisted-by: Claude Opus 4.6
stanleygambarin
force-pushed
the
extended_llt_fix
branch
from
April 14, 2026 15:57
65cf3a1 to
92cb18b
Compare
michalpaszkowski
approved these changes
Apr 14, 2026
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/164/builds/20613 Here is the relevant piece of the build log for the reference |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The GlobalISel uses a lookup table to map LLTs which is constructed
prior to initialization of extended LLT functionality, resulting in
ANY_SCALAR entries. During instruction selection, a hash-based
lookup is done on actual INTEGER/FLOAT LLTs. But hash values of
ANY_SCALAR do not match those of INTEGER/FLOAT, causing a failure.
Workaround is the use constexpr LLT, which encodes INTEGER/FLOAT LLT.
Assisted-by: Claude Opus 4.6