Skip to content

Commit f17547b

Browse files
cshungcshung
committed
WIP - Code Review Feedback
1 parent 43649de commit f17547b

File tree

15 files changed

+106
-176
lines changed

15 files changed

+106
-176
lines changed

‎src/coreclr/inc/readytorun.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ enum class ReadyToRunSectionType : uint32_t
8787
PgoInstrumentationData = 117, // Added in V5.2
8888
ManifestAssemblyMvids = 118, // Added in V5.3
8989
CrossModuleInlineInfo = 119, // Added in V6.2
90-
HotColdMap = 120, // Added in V6.3
90+
HotColdMap = 120, // Added in V8.0
9191

9292
// If you add a new section consider whether it is a breaking or non-breaking change.
9393
// Usually it is non-breaking, but if it is preferable to have older runtimes fail

‎src/coreclr/tools/Common/Internal/Runtime/ModuleHeaders.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public enum ReadyToRunSectionType
6969
PgoInstrumentationData = 117, // Added in 5.2
7070
ManifestAssemblyMvids = 118, // Added in 5.3
7171
CrossModuleInlineInfo = 119, // Added in 6.3
72-
HotColdMap = 120, // Added in 6.3
72+
HotColdMap = 120, // Added in 8.0
7373

7474
//
7575
// NativeAOT ReadyToRun sections

‎src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ private void PublishCode()
484484
alignment,
485485
new ISymbolDefinitionNode[] { _methodColdCodeNode });
486486
_methodColdCodeNode.SetCode(coldObjectData);
487-
_methodCodeNode.SetColdCodeNode(_methodColdCodeNode);
487+
_methodCodeNode.ColdCodeNode = _methodColdCodeNode;
488488
}
489489
#endif
490490

‎src/coreclr/tools/Common/TypeSystem/Common/TargetDetails.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,5 +332,10 @@ public int MaxHomogeneousAggregateElementCount
332332
return 4;
333333
}
334334
}
335+
336+
/// <summary>
337+
/// CodeDelta - encapsulate the fact that ARM requires a thumb bit
338+
/// </summary>
339+
public int CodeDelta { get => (Architecture == TargetArchitecture.ARM) ? 1 : 0; }
335340
}
336341
}

‎src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelayLoadHelperImport.cs

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,14 +64,8 @@ public override void EncodeData(ref ObjectDataBuilder dataBuilder, NodeFactory f
6464
{
6565
// This needs to be an empty target pointer since it will be filled in with Module*
6666
// when loaded by CoreCLR
67-
int codeDelta = 0;
68-
if (factory.Target.Architecture == TargetArchitecture.ARM)
69-
{
70-
// THUMB_CODE
71-
codeDelta = 1;
72-
}
7367
dataBuilder.EmitReloc(_delayLoadHelper,
74-
factory.Target.PointerSize == 4 ? RelocType.IMAGE_REL_BASED_HIGHLOW : RelocType.IMAGE_REL_BASED_DIR64, codeDelta);
68+
factory.Target.PointerSize == 4 ? RelocType.IMAGE_REL_BASED_HIGHLOW : RelocType.IMAGE_REL_BASED_DIR64, factory.Target.CodeDelta);
7569
}
7670

7771
public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory factory)

‎src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/HotColdMapNode.cs

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,29 @@
44
using System;
55

66
using Internal.Text;
7+
using System.Diagnostics;
78

89
namespace ILCompiler.DependencyAnalysis.ReadyToRun
910
{
1011
public class HotColdMapNode : HeaderTableNode
1112
{
12-
public uint[] mapping;
13+
private uint[] _mapping;
1314

1415
public HotColdMapNode(NodeFactory nodeFactory)
1516
: base(nodeFactory.Target)
1617
{
1718
}
1819

20+
public uint[] Mapping
21+
{
22+
get => _mapping;
23+
set
24+
{
25+
Debug.Assert(_mapping == null);
26+
_mapping = value;
27+
}
28+
}
29+
1930
public override int ClassCode => 28963035;
2031

2132
public override void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb)
@@ -32,7 +43,7 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
3243

3344
ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly);
3445
builder.AddSymbol(this);
35-
foreach (uint m in this.mapping)
46+
foreach (uint m in this._mapping)
3647
{
3748
builder.EmitUInt(m);
3849
}

‎src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodGCInfoNode.cs

Lines changed: 49 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ protected override void OnMarked(NodeFactory factory)
4040
public int[] CalculateFuncletOffsets(NodeFactory factory)
4141
{
4242
int coldCodeUnwindInfoCount = 0;
43-
if (_methodNode.GetColdCodeNode() != null)
43+
if (_methodNode.ColdCodeNode != null)
4444
{
4545
coldCodeUnwindInfoCount = _methodNode.ColdFrameInfos.Length;
4646
}
@@ -68,7 +68,7 @@ public int[] CalculateFuncletOffsets(NodeFactory factory)
6868
}
6969
}
7070

71-
if (_methodNode.GetColdCodeNode() != null)
71+
if (_methodNode.ColdCodeNode != null)
7272
{
7373
// TODO: Take a look at deduplicatedResult
7474
for (int frameInfoIndex = 0; frameInfoIndex < _methodNode.ColdFrameInfos.Length; frameInfoIndex++)
@@ -156,104 +156,68 @@ private IEnumerable<GCInfoComponent> EncodeDataCore(NodeFactory factory)
156156
{
157157
TargetArchitecture targetArch = factory.Target.Architecture;
158158

159-
for (int frameInfoIndex = 0; frameInfoIndex < _methodNode.FrameInfos.Length; frameInfoIndex++)
159+
int numHotFrameInfos = _methodNode.FrameInfos.Length;
160+
int numFrameInfos = numHotFrameInfos;
161+
if (_methodNode.ColdCodeNode != null)
160162
{
161-
byte[] unwindInfo = _methodNode.FrameInfos[frameInfoIndex].BlobData;
162-
163-
if (targetArch == TargetArchitecture.X64)
164-
{
165-
// On Amd64, patch the first byte of the unwind info by setting the flags to EHANDLER | UHANDLER
166-
// as that's what CoreCLR does (zapcode.cpp, ZapUnwindData::Save).
167-
const byte UNW_FLAG_EHANDLER = 1;
168-
const byte UNW_FLAG_UHANDLER = 2;
169-
const byte FlagsShift = 3;
163+
numFrameInfos += _methodNode.ColdFrameInfos.Length;
164+
}
170165

171-
unwindInfo[0] |= (byte)((UNW_FLAG_EHANDLER | UNW_FLAG_UHANDLER) << FlagsShift);
172-
}
173-
else if ((targetArch == TargetArchitecture.ARM) || (targetArch == TargetArchitecture.ARM64) || (targetArch == TargetArchitecture.LoongArch64))
166+
const byte UNW_FLAG_EHANDLER = 1;
167+
const byte UNW_FLAG_UHANDLER = 2;
168+
const byte UNW_FLAG_CHAININFO = 4;
169+
const byte FlagsShift = 3;
170+
171+
for (int frameInfoIndex = 0; frameInfoIndex < numFrameInfos; frameInfoIndex++)
172+
{
173+
FrameInfo frameInfo = (frameInfoIndex >= numHotFrameInfos) ?
174+
_methodNode.ColdFrameInfos[frameInfoIndex - numHotFrameInfos] :
175+
_methodNode.FrameInfos[frameInfoIndex];
176+
byte[] unwindInfo = frameInfo.BlobData;
177+
if (unwindInfo == null)
174178
{
175-
// Set the 'X' bit to indicate that there is a personality routine associated with this method
176-
unwindInfo[2] |= 1 << 4;
179+
// Chain unwind info
180+
byte[] header = new byte[4];
181+
int i = 0;
182+
header[i++] = 1 + (UNW_FLAG_CHAININFO << FlagsShift); // Version = 1, UNW_FLAG_CHAININFO
183+
header[i++] = 0; // SizeOfProlog = 0
184+
header[i++] = 0; // CountOfCode = 0
185+
header[i++] = _methodNode.FrameInfos[0].BlobData[3]; // Copying frame and frame offset from main function
186+
yield return new GCInfoComponent(header);
187+
yield return new GCInfoComponent(_methodNode, 0);
188+
yield return new GCInfoComponent(_methodNode, _methodNode.Size);
189+
// TODO: Is this correct?
190+
yield return new GCInfoComponent(factory.RuntimeFunctionsGCInfo.StartSymbol, this.OffsetFromBeginningOfArray);
177191
}
178-
179-
yield return new GCInfoComponent(unwindInfo);
180-
181-
if (targetArch != TargetArchitecture.X86)
192+
else
182193
{
183-
bool isFilterFunclet = (_methodNode.FrameInfos[frameInfoIndex].Flags & FrameInfoFlags.Filter) != 0;
184-
ISymbolNode personalityRoutine = (isFilterFunclet ? factory.FilterFuncletPersonalityRoutine : factory.PersonalityRoutine);
185-
int codeDelta = 0;
186-
if (targetArch == TargetArchitecture.ARM)
194+
if (targetArch == TargetArchitecture.X64)
187195
{
188-
// THUMB_CODE
189-
codeDelta = 1;
196+
// On Amd64, patch the first byte of the unwind info by setting the flags to EHANDLER | UHANDLER
197+
// as that's what CoreCLR does (zapcode.cpp, ZapUnwindData::Save).
198+
unwindInfo[0] |= (byte)((UNW_FLAG_EHANDLER | UNW_FLAG_UHANDLER) << FlagsShift);
190199
}
191-
yield return new GCInfoComponent(personalityRoutine, codeDelta);
192-
}
193-
194-
if (frameInfoIndex == 0 && _methodNode.GCInfo != null)
195-
{
196-
yield return new GCInfoComponent(_methodNode.GCInfo);
197-
}
198-
}
199-
#if READYTORUN
200-
if (_methodNode.GetColdCodeNode() != null)
201-
{
202-
for (int frameInfoIndex = 0; frameInfoIndex < _methodNode.ColdFrameInfos.Length; frameInfoIndex++)
203-
{
204-
byte[] unwindInfo = _methodNode.ColdFrameInfos[frameInfoIndex].BlobData;
205-
206-
if (unwindInfo == null)
200+
else if ((targetArch == TargetArchitecture.ARM) || (targetArch == TargetArchitecture.ARM64) || (targetArch == TargetArchitecture.LoongArch64))
207201
{
208-
// Chain unwind info
209-
byte[] header = new byte[4];
210-
int i = 0;
211-
header[i++] = 1 + (4 << 3); // Version = 1, UNW_FLAG_CHAININFO
212-
header[i++] = 0; // SizeOfProlog = 0
213-
header[i++] = 0; // CountOfCode = 0
214-
header[i++] = _methodNode.FrameInfos[0].BlobData[3]; // Copying frame and frame offset from main function
215-
yield return new GCInfoComponent(header);
216-
yield return new GCInfoComponent(_methodNode, 0);
217-
yield return new GCInfoComponent(_methodNode, _methodNode.Size);
218-
// TODO: Is this correct?
219-
yield return new GCInfoComponent(factory.RuntimeFunctionsGCInfo.StartSymbol, this.OffsetFromBeginningOfArray);
202+
// Set the 'X' bit to indicate that there is a personality routine associated with this method
203+
unwindInfo[2] |= 1 << 4;
220204
}
221-
else
222-
{
223-
if (targetArch == TargetArchitecture.X64)
224-
{
225-
// On Amd64, patch the first byte of the unwind info by setting the flags to EHANDLER | UHANDLER
226-
// as that's what CoreCLR does (zapcode.cpp, ZapUnwindData::Save).
227-
const byte UNW_FLAG_EHANDLER = 1;
228-
const byte UNW_FLAG_UHANDLER = 2;
229-
const byte FlagsShift = 3;
230205

231-
unwindInfo[0] |= (byte)((UNW_FLAG_EHANDLER | UNW_FLAG_UHANDLER) << FlagsShift);
232-
}
233-
else if ((targetArch == TargetArchitecture.ARM) || (targetArch == TargetArchitecture.ARM64))
234-
{
235-
// Set the 'X' bit to indicate that there is a personality routine associated with this method
236-
unwindInfo[2] |= 1 << 4;
237-
}
206+
yield return new GCInfoComponent(unwindInfo);
238207

239-
yield return new GCInfoComponent(unwindInfo);
208+
if (targetArch != TargetArchitecture.X86)
209+
{
210+
bool isFilterFunclet = (frameInfo.Flags & FrameInfoFlags.Filter) != 0;
211+
ISymbolNode personalityRoutine = (isFilterFunclet ? factory.FilterFuncletPersonalityRoutine : factory.PersonalityRoutine);
212+
yield return new GCInfoComponent(personalityRoutine, factory.Target.CodeDelta);
213+
}
240214

241-
if (targetArch != TargetArchitecture.X86)
242-
{
243-
bool isFilterFunclet = (_methodNode.ColdFrameInfos[frameInfoIndex].Flags & FrameInfoFlags.Filter) != 0;
244-
ISymbolNode personalityRoutine = (isFilterFunclet ? factory.FilterFuncletPersonalityRoutine : factory.PersonalityRoutine);
245-
int codeDelta = 0;
246-
if (targetArch == TargetArchitecture.ARM)
247-
{
248-
// THUMB_CODE
249-
codeDelta = 1;
250-
}
251-
yield return new GCInfoComponent(personalityRoutine, codeDelta);
252-
}
215+
if (frameInfoIndex == 0 && _methodNode.GCInfo != null)
216+
{
217+
yield return new GCInfoComponent(_methodNode.GCInfo);
253218
}
254219
}
255220
}
256-
#endif
257221
}
258222

259223
class MethodGCInfoNodeDeduplicatingComparer : IEqualityComparer<MethodGCInfoNode>

‎src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodWithGCInfo.cs

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,8 @@ public class MethodWithGCInfo : ObjectNode, IMethodBodyNode, ISymbolDefinitionNo
1919
private readonly MethodDesc _method;
2020

2121
private ObjectData _methodCode;
22-
#if READYTORUN
23-
private MethodColdCodeNode _methodColdCodeNode;
24-
#endif
2522
private FrameInfo[] _frameInfos;
26-
#if READYTORUN
2723
private FrameInfo[] _coldFrameInfos;
28-
#endif
2924
private byte[] _gcInfo;
3025
private ObjectData _ehInfo;
3126
private byte[] _debugLocInfos;
@@ -50,9 +45,7 @@ protected override void OnMarked(NodeFactory context)
5045
{
5146
SetCode(new ObjectNode.ObjectData(Array.Empty<byte>(), null, 1, Array.Empty<ISymbolDefinitionNode>()));
5247
InitializeFrameInfos(Array.Empty<FrameInfo>());
53-
#if READYTORUN
5448
InitializeColdFrameInfos(Array.Empty<FrameInfo>());
55-
#endif
5649
}
5750
_lateTriggeredCompilation = context.CompilationCurrentPhase != 0;
5851
RegisterInlineeModuleIndices(context);
@@ -139,16 +132,15 @@ public int Compare(FixupCell a, FixupCell b)
139132
}
140133
}
141134

142-
public MethodColdCodeNode GetColdCodeNode() => _methodColdCodeNode;
135+
public MethodColdCodeNode ColdCodeNode { get; set; }
143136

144137
public byte[] GetFixupBlob(NodeFactory factory)
145138
{
146139
Relocation[] relocations = GetData(factory, relocsOnly: true).Relocs;
147140

148-
#if READYTORUN
149-
if (_methodColdCodeNode != null)
141+
if (ColdCodeNode != null)
150142
{
151-
Relocation[] coldRelocations = _methodColdCodeNode.GetData(factory, relocsOnly: true).Relocs;
143+
Relocation[] coldRelocations = ColdCodeNode.GetData(factory, relocsOnly: true).Relocs;
152144
if (relocations == null)
153145
{
154146
relocations = coldRelocations;
@@ -158,7 +150,6 @@ public byte[] GetFixupBlob(NodeFactory factory)
158150
relocations = Enumerable.Concat(relocations, coldRelocations).ToArray();
159151
}
160152
}
161-
#endif
162153

163154
if (relocations == null)
164155
{
@@ -271,9 +262,9 @@ protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFact
271262
{
272263
DependencyList dependencyList = new DependencyList(new DependencyListEntry[] { new DependencyListEntry(GCInfoNode, "Unwind & GC info") });
273264

274-
if (this.GetColdCodeNode() != null)
265+
if (this.ColdCodeNode != null)
275266
{
276-
dependencyList.Add(this.GetColdCodeNode(), "cold");
267+
dependencyList.Add(this.ColdCodeNode, "cold");
277268
}
278269

279270
foreach (ISymbolNode node in _fixups)
@@ -312,9 +303,7 @@ public override ObjectNodeSection Section
312303

313304
public FrameInfo[] FrameInfos => _frameInfos;
314305

315-
#if READYTORUN
316306
public FrameInfo[] ColdFrameInfos => _coldFrameInfos;
317-
#endif
318307

319308
public byte[] GCInfo => _gcInfo;
320309
public ObjectData EHInfo => _ehInfo;
@@ -337,14 +326,12 @@ public void InitializeFrameInfos(FrameInfo[] frameInfos)
337326
}
338327
}
339328

340-
#if READYTORUN
341329
public void InitializeColdFrameInfos(FrameInfo[] coldFrameInfos)
342330
{
343331
Debug.Assert(_coldFrameInfos == null);
344332
_coldFrameInfos = coldFrameInfos;
345333
// TODO: x86 (see InitializeFrameInfos())
346334
}
347-
#endif
348335

349336
public void InitializeGCInfo(byte[] gcInfo)
350337
{
@@ -403,12 +390,5 @@ public void InitializeInliningInfo(MethodDesc[] inlinedMethods, NodeFactory fact
403390
public override bool ShouldSkipEmittingObjectNode(NodeFactory factory) => IsEmpty;
404391

405392
public override string ToString() => _method.ToString();
406-
407-
#if READYTORUN
408-
public void SetColdCodeNode(MethodColdCodeNode methodColdCodeNode)
409-
{
410-
_methodColdCodeNode = methodColdCodeNode;
411-
}
412-
#endif
413393
}
414394
}

0 commit comments

Comments
 (0)