[HLSL][RootSignature] Implement ResourceRange as an IntervalMap
#140957
Conversation
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
|
@llvm/pr-subscribers-hlsl Author: Finn Plummer (inbelic) ChangesA resource range consists of a closed interval, For instance:
We want to provide an error diagnostic that there is an overlap in the required registers for each of the resources (an overlap in the resource ranges). The goal of this pr is to implement a structure to model a set of resource ranges and provide an api to detect any overlap over a set of resource ranges.
Note: it was also considered to implement this as an Part 1 of #129942 Full diff: https://github.com/llvm/llvm-project/pull/140957.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index 98fa5f09429e3..3e5054566052b 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -15,6 +15,7 @@
#define LLVM_FRONTEND_HLSL_HLSLROOTSIGNATURE_H
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/IntervalMap.h"
#include "llvm/Support/DXILABI.h"
#include "llvm/Support/raw_ostream.h"
#include <variant>
@@ -198,6 +199,61 @@ class MetadataBuilder {
SmallVector<Metadata *> GeneratedMetadata;
};
+// RangeInfo holds the information to correctly construct a ResourceRange
+// and retains this information to be used for displaying a better diagnostic
+struct RangeInfo {
+ const static uint32_t Unbounded = static_cast<uint32_t>(-1);
+
+ uint32_t LowerBound;
+ uint32_t UpperBound;
+};
+
+class ResourceRange {
+public:
+ using IMap = llvm::IntervalMap<uint32_t, const RangeInfo *, 16,
+ llvm::IntervalMapInfo<uint32_t>>;
+
+private:
+ IMap Intervals;
+
+public:
+ ResourceRange(IMap::Allocator &Allocator) : Intervals(IMap(Allocator)) {}
+
+ // Returns a reference to the first RangeInfo that overlaps with
+ // [Info.LowerBound;Info.UpperBound], or, std::nullopt if there is no overlap
+ std::optional<const RangeInfo *> getOverlapping(const RangeInfo &Info) const;
+
+ // Return the mapped RangeInfo at X or nullptr if no mapping exists
+ const RangeInfo *lookup(uint32_t X) const;
+
+ // Insert the required (sub-)intervals such that the interval of [a;b] =
+ // [Info.LowerBound, Info.UpperBound] is covered and points to a valid
+ // RangeInfo &.
+ //
+ // For instance consider the following chain of inserting RangeInfos with the
+ // intervals denoting the Lower/Upper-bounds:
+ //
+ // A = [0;2]
+ // insert(A) -> false
+ // intervals: [0;2] -> &A
+ // B = [5;7]
+ // insert(B) -> false
+ // intervals: [0;2] -> &A, [5;7] -> &B
+ // C = [4;7]
+ // insert(C) -> true
+ // intervals: [0;2] -> &A, [4;7] -> &C
+ // D = [1;5]
+ // insert(D) -> true
+ // intervals: [0;2] -> &A, [3;3] -> &D, [4;7] -> &C
+ // E = [0;unbounded]
+ // insert(E) -> true
+ // intervals: [0;unbounded] -> E
+ //
+ // Returns if the first overlapping range when inserting
+ // (same return as getOverlapping)
+ std::optional<const RangeInfo *> insert(const RangeInfo &Info);
+};
+
} // namespace rootsig
} // namespace hlsl
} // namespace llvm
diff --git a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
index abf076944b273..76fe09e4dc3ee 100644
--- a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
+++ b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
@@ -222,6 +222,67 @@ MDNode *MetadataBuilder::BuildDescriptorTableClause(
});
}
+std::optional<const RangeInfo *>
+ResourceRange::getOverlapping(const RangeInfo &Info) const {
+ IMap::const_iterator Interval = Intervals.find(Info.LowerBound);
+ if (!Interval.valid() || Info.UpperBound < Interval.start())
+ return std::nullopt;
+ return Interval.value();
+}
+
+const RangeInfo *ResourceRange::lookup(uint32_t X) const {
+ return Intervals.lookup(X, nullptr);
+}
+
+std::optional<const RangeInfo *> ResourceRange::insert(const RangeInfo &Info) {
+ uint32_t LowerBound = Info.LowerBound;
+ uint32_t UpperBound = Info.UpperBound;
+
+ std::optional<const RangeInfo *> Res = std::nullopt;
+ IMap::iterator Interval = Intervals.begin();
+
+ while (true) {
+ if (UpperBound < LowerBound)
+ break;
+
+ Interval.advanceTo(LowerBound);
+ if (!Interval.valid()) // No interval found
+ break;
+
+ // Let Interval = [x;y] and [LowerBound;UpperBound] = [a;b] and note that
+ // a <= y implicitly from Intervals.find(LowerBound)
+ if (UpperBound < Interval.start())
+ break; // found interval does not overlap with inserted one
+
+ if (!Res.has_value()) // Update to be the first found intersection
+ Res = Interval.value();
+
+ if (Interval.start() <= LowerBound && UpperBound <= Interval.stop()) {
+ // x <= a <= b <= y implies that [a;b] is covered by [x;y]
+ // -> so we don't need to insert this, report an overlap
+ return Res;
+ } else if (LowerBound <= Interval.start() &&
+ Interval.stop() <= UpperBound) {
+ // a <= x <= y <= b implies that [x;y] is covered by [a;b]
+ // -> so remove the existing interval that we will cover with the
+ // overwrite
+ Interval.erase();
+ } else if (LowerBound < Interval.start() && UpperBound <= Interval.stop()) {
+ // a < x <= b <= y implies that [a; x] is not covered but [x;b] is
+ // -> so set b = x - 1 such that [a;x-1] is now the interval to insert
+ UpperBound = Interval.start() - 1;
+ } else if (Interval.start() <= LowerBound && Interval.stop() < UpperBound) {
+ // a < x <= b <= y implies that [y; b] is not covered but [a;y] is
+ // -> so set a = y + 1 such that [y+1;b] is now the interval to insert
+ LowerBound = Interval.stop() + 1;
+ }
+ }
+
+ assert(LowerBound <= UpperBound && "Attempting to insert an empty interval");
+ Intervals.insert(LowerBound, UpperBound, &Info);
+ return Res;
+}
+
} // namespace rootsig
} // namespace hlsl
} // namespace llvm
diff --git a/llvm/unittests/Frontend/CMakeLists.txt b/llvm/unittests/Frontend/CMakeLists.txt
index 2119642769e3d..4048143b36819 100644
--- a/llvm/unittests/Frontend/CMakeLists.txt
+++ b/llvm/unittests/Frontend/CMakeLists.txt
@@ -12,6 +12,7 @@ set(LLVM_LINK_COMPONENTS
add_llvm_unittest(LLVMFrontendTests
HLSLRootSignatureDumpTest.cpp
+ HLSLRootSignatureRangesTest.cpp
OpenACCTest.cpp
OpenMPContextTest.cpp
OpenMPIRBuilderTest.cpp
diff --git a/llvm/unittests/Frontend/HLSLRootSignatureRangesTest.cpp b/llvm/unittests/Frontend/HLSLRootSignatureRangesTest.cpp
new file mode 100644
index 0000000000000..f1e8be06ad547
--- /dev/null
+++ b/llvm/unittests/Frontend/HLSLRootSignatureRangesTest.cpp
@@ -0,0 +1,177 @@
+//===------ HLSLRootSignatureRangeTest.cpp - RootSignature Range tests ----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Frontend/HLSL/HLSLRootSignature.h"
+#include "gtest/gtest.h"
+
+using namespace llvm::hlsl::rootsig;
+
+namespace {
+
+TEST(HLSLRootSignatureTest, NoOverlappingInsertTests) {
+ // Ensures that there is never a reported overlap
+ ResourceRange::IMap::Allocator Allocator;
+ ResourceRange Range(Allocator);
+
+ RangeInfo A;
+ A.LowerBound = 0;
+ A.UpperBound = 3;
+ EXPECT_EQ(Range.insert(A), std::nullopt);
+
+ RangeInfo B;
+ B.LowerBound = 4;
+ B.UpperBound = 7;
+ EXPECT_EQ(Range.insert(B), std::nullopt);
+
+ RangeInfo C;
+ C.LowerBound = 10;
+ C.UpperBound = RangeInfo::Unbounded;
+ EXPECT_EQ(Range.insert(C), std::nullopt);
+
+ // A = [0;3]
+ EXPECT_EQ(Range.lookup(0), &A);
+ EXPECT_EQ(Range.lookup(2), &A);
+ EXPECT_EQ(Range.lookup(3), &A);
+
+ // B = [4;7]
+ EXPECT_EQ(Range.lookup(4), &B);
+ EXPECT_EQ(Range.lookup(5), &B);
+ EXPECT_EQ(Range.lookup(7), &B);
+
+ EXPECT_EQ(Range.lookup(8), nullptr);
+ EXPECT_EQ(Range.lookup(9), nullptr);
+
+ // C = [10;unbounded]
+ EXPECT_EQ(Range.lookup(10), &C);
+ EXPECT_EQ(Range.lookup(42), &C);
+ EXPECT_EQ(Range.lookup(98237423), &C);
+ EXPECT_EQ(Range.lookup(RangeInfo::Unbounded), &C);
+}
+
+TEST(HLSLRootSignatureTest, SingleOverlappingInsertTests) {
+ // Ensures that we correctly report an overlap when we insert a range that
+ // overlaps with one other range but does not cover (replace) it
+ ResourceRange::IMap::Allocator Allocator;
+ ResourceRange Range(Allocator);
+
+ RangeInfo A;
+ A.LowerBound = 1;
+ A.UpperBound = 5;
+ EXPECT_EQ(Range.insert(A), std::nullopt);
+
+ RangeInfo B;
+ B.LowerBound = 0;
+ B.UpperBound = 2;
+ EXPECT_EQ(Range.insert(B).value(), &A);
+
+ RangeInfo C;
+ C.LowerBound = 4;
+ C.UpperBound = RangeInfo::Unbounded;
+ EXPECT_EQ(Range.insert(C).value(), &A);
+
+ // A = [1;5]
+ EXPECT_EQ(Range.lookup(1), &A);
+ EXPECT_EQ(Range.lookup(2), &A);
+ EXPECT_EQ(Range.lookup(3), &A);
+ EXPECT_EQ(Range.lookup(4), &A);
+ EXPECT_EQ(Range.lookup(5), &A);
+
+ // B = [0;0]
+ EXPECT_EQ(Range.lookup(0), &B);
+
+ // C = [6; unbounded]
+ EXPECT_EQ(Range.lookup(6), &C);
+ EXPECT_EQ(Range.lookup(RangeInfo::Unbounded), &C);
+}
+
+TEST(HLSLRootSignatureTest, MultipleOverlappingInsertTests) {
+ // Ensures that we correctly report an overlap when inserted range
+ // overlaps more than one range and it does not cover (replace) either
+ // range. In this case it will just fill in the interval between the two
+ ResourceRange::IMap::Allocator Allocator;
+ ResourceRange Range(Allocator);
+
+ RangeInfo A;
+ A.LowerBound = 0;
+ A.UpperBound = 2;
+ EXPECT_EQ(Range.insert(A), std::nullopt);
+
+ RangeInfo B;
+ B.LowerBound = 4;
+ B.UpperBound = 6;
+ EXPECT_EQ(Range.insert(B), std::nullopt);
+
+ RangeInfo C;
+ C.LowerBound = 1;
+ C.UpperBound = 5;
+ EXPECT_EQ(Range.insert(C).value(), &A);
+
+ // A = [0;2]
+ EXPECT_EQ(Range.lookup(0), &A);
+ EXPECT_EQ(Range.lookup(1), &A);
+ EXPECT_EQ(Range.lookup(2), &A);
+
+ // B = [4;6]
+ EXPECT_EQ(Range.lookup(4), &B);
+ EXPECT_EQ(Range.lookup(5), &B);
+ EXPECT_EQ(Range.lookup(6), &B);
+
+ // C = [3;3]
+ EXPECT_EQ(Range.lookup(3), &C);
+}
+
+TEST(HLSLRootSignatureTest, CoverInsertTests) {
+ // Ensures that we correctly report an overlap when inserted range
+ // covers one or more ranges
+ ResourceRange::IMap::Allocator Allocator;
+ ResourceRange Range(Allocator);
+
+ RangeInfo A;
+ A.LowerBound = 0;
+ A.UpperBound = 2;
+ EXPECT_EQ(Range.insert(A), std::nullopt);
+
+ RangeInfo B;
+ B.LowerBound = 4;
+ B.UpperBound = 5;
+ EXPECT_EQ(Range.insert(B), std::nullopt);
+
+ // Covers B
+ RangeInfo C;
+ C.LowerBound = 4;
+ C.UpperBound = 6;
+ EXPECT_EQ(Range.insert(C).value(), &B);
+
+ // A = [0;2]
+ // C = [4;6] <- covers reference to B
+ EXPECT_EQ(Range.lookup(0), &A);
+ EXPECT_EQ(Range.lookup(1), &A);
+ EXPECT_EQ(Range.lookup(2), &A);
+ EXPECT_EQ(Range.lookup(3), nullptr);
+ EXPECT_EQ(Range.lookup(4), &C);
+ EXPECT_EQ(Range.lookup(5), &C);
+ EXPECT_EQ(Range.lookup(6), &C);
+
+ // Covers all other ranges
+ RangeInfo D;
+ D.LowerBound = 0;
+ D.UpperBound = 7;
+ EXPECT_EQ(Range.insert(D).value(), &A);
+
+ // D = [0;7] <- Covers reference to A and C
+ EXPECT_EQ(Range.lookup(0), &D);
+ EXPECT_EQ(Range.lookup(1), &D);
+ EXPECT_EQ(Range.lookup(2), &D);
+ EXPECT_EQ(Range.lookup(3), &D);
+ EXPECT_EQ(Range.lookup(4), &D);
+ EXPECT_EQ(Range.lookup(5), &D);
+ EXPECT_EQ(Range.lookup(6), &D);
+ EXPECT_EQ(Range.lookup(7), &D);
+}
+
+} // namespace
|
Icohedron
reviewed
May 23, 2025
Icohedron
approved these changes
May 23, 2025
6 tasks
bogner
approved these changes
May 29, 2025
inbelic
force-pushed
the
inbelic/rs-resource-range-intervals
branch
from
f4b94ca to
ee63c2e
Compare
6 tasks
- use MapT instead of IMap - use ~0u for conciseness
inbelic
force-pushed
the
inbelic/rs-resource-range-intervals
branch
from
87241ac to
ab4bbe0
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/20329 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/11706 Here is the relevant piece of the build log for the reference |
fschlimb
pushed a commit
to fschlimb/llvm-project
that referenced
this pull request
Jun 18, 2025
…lvm#140957) A resource range consists of a closed interval, `[a;b]`, denoting which shader registers it is bound to. For instance: - `CBV(b1)` corresponds to the resource range of `[1;1]` - `CBV(b0, numDescriptors = 3)` likewise to `[0;2]` We want to provide an error diagnostic when there is an overlap in the required registers (an overlap in the resource ranges). The goal of this pr is to implement a structure to model a set of resource ranges and provide an api to detect any overlap over a set of resource ranges. `ResourceRange` models this by implementing an `IntervalMap` to denote a mapping from an interval of registers back to a resource range. It allows for a new `ResourceRange` to be added to the mapping and it will report if and what the first overlap is. For the context of how this will be used in validation of a `RootSignatureDecl` please see the proceeding pull request here: llvm#140962. - Implements `ResourceRange` as an `IntervalMap` - Adds unit testing of the various `insert` scenarios Note: it was also considered to implement this as an `IntervalTree`, this would allow reporting of a diagnostic for each overlap that is encountered, as opposed to just the first. However, error generation of just reporting the first error is already rather verbose, and adding the additional diagnostics only made this worse. Part 1 of llvm#129942
inbelic
added a commit
that referenced
this pull request
Jun 20, 2025
…otDescriptors` (#140962) As was established [previously](#140957), we created a structure to model a resource range and to detect an overlap in a given set of these. However, a resource range only overlaps with another resource range if they have: - equivalent ResourceClass (SRV, UAV, CBuffer, Sampler) - equivalent resource name-space - overlapping shader visibility For instance, the following don't overlap even though they have the same register range: - `CBV(b0)` and `SRV(t0)` (different resource class) - `CBV(b0, space = 0)` and `CBV(b0, space = 1)` (different space) - `CBV(b0, visibility = Pixel)` and `CBV(b0, visibility = Domain)` (non-overlapping visibility) The first two clauses are naturally modelled by grouping all the `RangeInfo`s that have the equivalent `ResourceClass` and `Space` values together and check if there is any overlap on a `ResourceRange` for all these `RangeInfo`s. However, `Visibility` is not quite as easily mapped (`Visibility = All` would overlap with any other visibility). So we will instead need to track a `ResourceRange` for each of the `Visibility` types in a group. Then we can determine when inserting a range of the same group if it would overlap with any overlapping visibilities. The collection of `RangeInfo` for `RootDescriptor`s, sorting of the `RangeInfo`s into the groups and finally the insertion of each point into their respective `ResourceRange`s are implemented. Furthermore, we integrate this into `SemaHLSL` to provide a diagnostic for each entry function that uses the invalid root signature. - Implements collection of `RangeInfo` for `RootDescriptors` - Implements resource range validation in `SemaHLSL` - Add diagnostic testing of error production in `RootSignature-resource-ranges-err.hlsl` - Add testing to ensure no errors are raised in valid root signatures `RootSignature-resource-ranges.hlsl` Part 2 of #129942 A final pr will be produced to integrate the analysis of `DescriptorTable`, `StaticSampler` and `RootConstants` by defining how to construct the `RangeInfo` from their elements respectively.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 20, 2025
…ges for `RootDescriptors` (#140962) As was established [previously](llvm/llvm-project#140957), we created a structure to model a resource range and to detect an overlap in a given set of these. However, a resource range only overlaps with another resource range if they have: - equivalent ResourceClass (SRV, UAV, CBuffer, Sampler) - equivalent resource name-space - overlapping shader visibility For instance, the following don't overlap even though they have the same register range: - `CBV(b0)` and `SRV(t0)` (different resource class) - `CBV(b0, space = 0)` and `CBV(b0, space = 1)` (different space) - `CBV(b0, visibility = Pixel)` and `CBV(b0, visibility = Domain)` (non-overlapping visibility) The first two clauses are naturally modelled by grouping all the `RangeInfo`s that have the equivalent `ResourceClass` and `Space` values together and check if there is any overlap on a `ResourceRange` for all these `RangeInfo`s. However, `Visibility` is not quite as easily mapped (`Visibility = All` would overlap with any other visibility). So we will instead need to track a `ResourceRange` for each of the `Visibility` types in a group. Then we can determine when inserting a range of the same group if it would overlap with any overlapping visibilities. The collection of `RangeInfo` for `RootDescriptor`s, sorting of the `RangeInfo`s into the groups and finally the insertion of each point into their respective `ResourceRange`s are implemented. Furthermore, we integrate this into `SemaHLSL` to provide a diagnostic for each entry function that uses the invalid root signature. - Implements collection of `RangeInfo` for `RootDescriptors` - Implements resource range validation in `SemaHLSL` - Add diagnostic testing of error production in `RootSignature-resource-ranges-err.hlsl` - Add testing to ensure no errors are raised in valid root signatures `RootSignature-resource-ranges.hlsl` Part 2 of llvm/llvm-project#129942 A final pr will be produced to integrate the analysis of `DescriptorTable`, `StaticSampler` and `RootConstants` by defining how to construct the `RangeInfo` from their elements respectively.
Jaddyen
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jun 23, 2025
…otDescriptors` (llvm#140962) As was established [previously](llvm#140957), we created a structure to model a resource range and to detect an overlap in a given set of these. However, a resource range only overlaps with another resource range if they have: - equivalent ResourceClass (SRV, UAV, CBuffer, Sampler) - equivalent resource name-space - overlapping shader visibility For instance, the following don't overlap even though they have the same register range: - `CBV(b0)` and `SRV(t0)` (different resource class) - `CBV(b0, space = 0)` and `CBV(b0, space = 1)` (different space) - `CBV(b0, visibility = Pixel)` and `CBV(b0, visibility = Domain)` (non-overlapping visibility) The first two clauses are naturally modelled by grouping all the `RangeInfo`s that have the equivalent `ResourceClass` and `Space` values together and check if there is any overlap on a `ResourceRange` for all these `RangeInfo`s. However, `Visibility` is not quite as easily mapped (`Visibility = All` would overlap with any other visibility). So we will instead need to track a `ResourceRange` for each of the `Visibility` types in a group. Then we can determine when inserting a range of the same group if it would overlap with any overlapping visibilities. The collection of `RangeInfo` for `RootDescriptor`s, sorting of the `RangeInfo`s into the groups and finally the insertion of each point into their respective `ResourceRange`s are implemented. Furthermore, we integrate this into `SemaHLSL` to provide a diagnostic for each entry function that uses the invalid root signature. - Implements collection of `RangeInfo` for `RootDescriptors` - Implements resource range validation in `SemaHLSL` - Add diagnostic testing of error production in `RootSignature-resource-ranges-err.hlsl` - Add testing to ensure no errors are raised in valid root signatures `RootSignature-resource-ranges.hlsl` Part 2 of llvm#129942 A final pr will be produced to integrate the analysis of `DescriptorTable`, `StaticSampler` and `RootConstants` by defining how to construct the `RangeInfo` from their elements respectively.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A resource range consists of a closed interval,
[a;b], denoting which shader registers it is bound to.For instance:
CBV(b1)corresponds to the resource range of[1;1]CBV(b0, numDescriptors = 3)likewise to[0;2]We want to provide an error diagnostic when there is an overlap in the required registers (an overlap in the resource ranges).
The goal of this pr is to implement a structure to model a set of resource ranges and provide an api to detect any overlap over a set of resource ranges.
ResourceRangemodels this by implementing anIntervalMapto denote a mapping from an interval of registers back to a resource range. It allows for a newResourceRangeto be added to the mapping and it will report if and what the first overlap is.For the context of how this will be used in validation of a
RootSignatureDeclplease see the proceeding pull request here: #140962.ResourceRangeas anIntervalMapinsertscenariosNote: it was also considered to implement this as an
IntervalTree, this would allow reporting of a diagnostic for each overlap that is encountered, as opposed to just the first. However, error generation of just reporting the first error is already rather verbose, and adding the additional diagnostics only made this worse.Part 1 of #129942