Page MenuHomePhabricator

tejohnson (Teresa Johnson)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 27 2015, 11:17 AM (374 w, 2 d)

Recent Activity

Today

tejohnson added a comment to D128863: Add switch to use "source_file_name" instead of Module ID for globally promoted local.

During LTO a local promoted to a global gets a unique suffix based partially on the Module's ID.

Wed, Jun 29, 4:48 PM · Restricted Project, Restricted Project
tejohnson accepted D128771: [ThinLTO][test] Add tests for emitting files in-process.

lgtm

Wed, Jun 29, 1:47 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D128142: [MemProf] Memprof profile matching and annotation.
Wed, Jun 29, 1:44 PM · Restricted Project, Restricted Project, Restricted Project
tejohnson requested review of D128854: [MemProf] Add memprof metadata related analysis utilities.
Wed, Jun 29, 1:43 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D128771: [ThinLTO][test] Add tests for emitting files in-process.
Wed, Jun 29, 9:33 AM · Restricted Project, Restricted Project
tejohnson added a comment to D128771: [ThinLTO][test] Add tests for emitting files in-process.

Thanks! Couple minor comments.

Wed, Jun 29, 9:32 AM · Restricted Project, Restricted Project

Fri, Jun 24

tejohnson added a comment to D127778: [LTO][ELF] Add selective --save-temps= option.

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

This is a good point, and for D127779 as well.

I defer to the both of you on that point. However, this patch and D127779 are more of an extension of --save-temps (in my opinion), and as such the tests are predicated on --save-temps being correct. I just assumed --save-temps was properly tested to begin with, but it seems to have only been tested within ELF/lto on a basic level. Would it be sufficient (for these 2 patches) to write llvm side tests for --save-temps as a separate patch?

Fri, Jun 24, 7:20 PM · Restricted Project, Restricted Project
tejohnson accepted D128564: [memprof] Return an error for unsupported symbolization..

lgtm with test comment suggestion

Fri, Jun 24, 4:47 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D127779: [LTO][ELF] Add --lto-exit-on option.
Fri, Jun 24, 10:49 AM · Restricted Project, Restricted Project
tejohnson added a comment to D127778: [LTO][ELF] Add selective --save-temps= option.

From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
(llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)

Fri, Jun 24, 10:42 AM · Restricted Project, Restricted Project

Wed, Jun 22

tejohnson accepted D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.

lgtm with one comment fix suggestion. Wait to see if @MaskRay has any additional comments.

Wed, Jun 22, 12:14 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D128141: [MemProf] Basic metadata support and verification.
Wed, Jun 22, 11:51 AM · Restricted Project, Restricted Project
tejohnson updated the diff for D128141: [MemProf] Basic metadata support and verification.

Address comments

Wed, Jun 22, 11:51 AM · Restricted Project, Restricted Project

Tue, Jun 21

tejohnson added inline comments to D127779: [LTO][ELF] Add --lto-exit-on option.
Tue, Jun 21, 7:18 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D127778: [LTO][ELF] Add selective --save-temps= option.
Tue, Jun 21, 7:18 PM · Restricted Project, Restricted Project
tejohnson added a comment to D116995: [gold] Ignore bitcode from sections inside object files.

@tejohnson Do the updates to the BitCodeFormat document look OK now?

Tue, Jun 21, 6:48 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D127777: [ThinLTO][ELF] Add --thinlto-emit-index-files option.
Tue, Jun 21, 12:14 PM · Restricted Project, Restricted Project

Sun, Jun 19

tejohnson requested review of D128143: [MemProf] Update metadata during inlining.
Sun, Jun 19, 10:19 AM · Restricted Project, Restricted Project
tejohnson requested review of D128142: [MemProf] Memprof profile matching and annotation.
Sun, Jun 19, 10:18 AM · Restricted Project, Restricted Project, Restricted Project
tejohnson requested review of D128141: [MemProf] Basic metadata support and verification.
Sun, Jun 19, 10:14 AM · Restricted Project, Restricted Project

Wed, Jun 15

tejohnson added a comment to D127876: [clang] Don't emit type test/assume for virtual classes that should never participate in WPD.

I prefer the way proposed here, which was what I initially intended to do in fact. @pcc, I recall from our early internal conversations about my proposal that you felt the new mechanism should apply to all classes, so that's why the final design did that. I do tend to think that if the source code has specifically annotated the classes with public LTO visibility it is better to enforce that always. I think revisiting this decision is worthwhile.

Wed, Jun 15, 11:32 AM · Restricted Project, Restricted Project

Tue, Jun 14

tejohnson accepted D127808: [memprof] Update the test comments to include -Wl,-no-pie.

lgtm

Tue, Jun 14, 7:16 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D127808: [memprof] Update the test comments to include -Wl,-no-pie.
Tue, Jun 14, 4:36 PM · Restricted Project, Restricted Project
tejohnson added a comment to D126089: [WPD] Try harder to find assumes through phis.

is this patch ok to submit at least as a short term workaround?

Chrome already has a short-term workaround, which is to disable opaque pointers, right? I think you can use that until the proper fix is developed.

Tue, Jun 14, 11:01 AM · Restricted Project, Restricted Project
tejohnson added a comment to D126089: [WPD] Try harder to find assumes through phis.

is this patch ok to submit at least as a short term workaround?

Tue, Jun 14, 9:52 AM · Restricted Project, Restricted Project

Fri, Jun 10

tejohnson added a comment to D126089: [WPD] Try harder to find assumes through phis.

(fyi made a couple of slight edits to my last comment for clarity)

Fri, Jun 10, 7:30 AM · Restricted Project, Restricted Project
tejohnson added a comment to D126089: [WPD] Try harder to find assumes through phis.

Thanks for the reproducer!

I think the root cause of the issue here is that the IR generated by Clang is wrong. llvm.type.test will only consider types that are local to the LTO unit, which means that it is incorrect to emit llvm.assume(llvm.type.test) for types A or B, because they have public LTO visibility, which means that definitions may reside outside of the LTO unit. The LowerTypeTests pass is therefore correctly replacing the llvm.type.tests with false, leading to the miscompilation seen here. The issue can also be seen if the [[clang::lto_visibility_public]] attribute is removed.

This is by design per the changes I made awhile back to delay the decision about visibility until LTO link time: https://groups.google.com/g/llvm-dev/c/6LfIiAo9g68/m/Ng1Xvw4WAwAJ
They should have public !vcall_visibility metadata by default, and since whole program visibility isn't being asserted here, there should be no devirt, etc.

The problem with this design is that it leaves the meaning of llvm.type.test undefined in the case where there are no globals that are a member of the type, because the !vcall_visibility is associated with the global. So there's no way to know whether the type argument was intended to refer to a type with hidden LTO visibility or one with public LTO visibility, if there are no member globals.

Since -lto-whole-program-visibility is a flag that affects the operation of the type tests globally, I think a better design would be to replace all llvm.type.test calls with true, and replaces llvm.type.checked.load with a regular load, if the flag is not passed.

We only perform WPD if there is a global, however. And then we subsequently remove these sequences if there is no global for a type id [1].

Maybe I am misunderstanding the concern though, can you give an example where this will go wrong?

I think Arthur's reproducer is exactly an example of where this will go wrong. Any IR which is not in the exact pattern expected by WPD (phi is just one example but who knows what passes will do in the future) will cause WPD to leave the IR in an incorrect form that will cause problems for LTT.

Fri, Jun 10, 6:19 AM · Restricted Project, Restricted Project

Thu, Jun 9

tejohnson added a comment to D126089: [WPD] Try harder to find assumes through phis.

Thanks for the reproducer!

I think the root cause of the issue here is that the IR generated by Clang is wrong. llvm.type.test will only consider types that are local to the LTO unit, which means that it is incorrect to emit llvm.assume(llvm.type.test) for types A or B, because they have public LTO visibility, which means that definitions may reside outside of the LTO unit. The LowerTypeTests pass is therefore correctly replacing the llvm.type.tests with false, leading to the miscompilation seen here. The issue can also be seen if the [[clang::lto_visibility_public]] attribute is removed.

This is by design per the changes I made awhile back to delay the decision about visibility until LTO link time: https://groups.google.com/g/llvm-dev/c/6LfIiAo9g68/m/Ng1Xvw4WAwAJ
They should have public !vcall_visibility metadata by default, and since whole program visibility isn't being asserted here, there should be no devirt, etc.

The problem with this design is that it leaves the meaning of llvm.type.test undefined in the case where there are no globals that are a member of the type, because the !vcall_visibility is associated with the global. So there's no way to know whether the type argument was intended to refer to a type with hidden LTO visibility or one with public LTO visibility, if there are no member globals.

Since -lto-whole-program-visibility is a flag that affects the operation of the type tests globally, I think a better design would be to replace all llvm.type.test calls with true, and replaces llvm.type.checked.load with a regular load, if the flag is not passed.

Thu, Jun 9, 2:00 PM · Restricted Project, Restricted Project
tejohnson added a comment to D126089: [WPD] Try harder to find assumes through phis.

Thanks for the reproducer!

I think the root cause of the issue here is that the IR generated by Clang is wrong. llvm.type.test will only consider types that are local to the LTO unit, which means that it is incorrect to emit llvm.assume(llvm.type.test) for types A or B, because they have public LTO visibility, which means that definitions may reside outside of the LTO unit. The LowerTypeTests pass is therefore correctly replacing the llvm.type.tests with false, leading to the miscompilation seen here. The issue can also be seen if the [[clang::lto_visibility_public]] attribute is removed.

Thu, Jun 9, 7:09 AM · Restricted Project, Restricted Project

Wed, Jun 1

tejohnson accepted D126834: [memprof] Update summary output..

lgtm

Wed, Jun 1, 6:19 PM · Restricted Project, Restricted Project
tejohnson accepted D126840: [memprof] Print out the segment information in YAML format..

lgtm

Wed, Jun 1, 6:18 PM · Restricted Project, Restricted Project
tejohnson added a comment to D126834: [memprof] Update summary output..

I think the 2 changes are unrelated so maybe they should be committed separately? Question below about one piece of info that changed.

Wed, Jun 1, 4:26 PM · Restricted Project, Restricted Project

May 27 2022

tejohnson added inline comments to D72386: [ThinLTO] pass UnrollLoops/VectorizeLoop/VectorizeSLP in CGOpts down to pass builder in ltobackend.
May 27 2022, 8:17 AM · Restricted Project, Restricted Project, Restricted Project

May 25 2022

tejohnson added a comment to D126089: [WPD] Try harder to find assumes through phis.

This case is supposed to be handled as a result of running the WPD pass early. See the comment near the top of PassBuilder::buildThinLTODefaultPipeline:

// These passes import type identifier resolutions for whole-program
// devirtualization and CFI. They must run early because other passes may
// disturb the specific instruction patterns that these passes look for,
// creating dependencies on resolutions that may not appear in the summary.
//
// For example, GVN may transform the pattern assume(type.test) appearing in
// two basic blocks into assume(phi(type.test, type.test)), which would
// transform a dependency on a WPD resolution into a dependency on a type
// identifier resolution for CFI.
//
// Also, WPD has access to more precise information than ICP and can
// devirtualize more effectively, so it should operate on the IR first.
//
// The WPD and LowerTypeTest passes need to run at -O0 to lower type
// metadata and intrinsics.

Is there some way that we can now end up with GVN or some other such pass running between when we create the summary and when we run WPD that's inserting a phi?

I thought the frontend emitted the type test/assume. I'm seeing them in the IR after the pre link step. CodeGenFunction::EmitTypeMetadataCodeForVCall() in clang emits a type test/assume.

Right, but the phi must be coming from somewhere else after summary generation if it isn't accounted for in the summary (if the phi was inserted before summary generation, the summary should end up with a dependency on a CFI type identifier resolution, instead of a WPD resolution). Can you trace where it is coming from?

I'm not super familiar with ThinLTO summaries, could you explain a bit more?
The assume(phi(type test, type test)) is coming from the output of the pre link. Then post link WPD looks at all type test usages and I think LTT does the same? So not sure where the summary is relevant.

The summary is meant to control what actions are done by the LTT and WPD passes later. For example here is the summary with an assume(phi(type.test, type.test)) which simulates what you told me that the phi comes from the output of the pre link:

> cat foo.ll 
define i1 @f() {
  br i1 undef, label %bb1, label %bb2

bb1:
  %p1 = call i1 @llvm.type.test(i8* null, metadata !"foo")
  br label %end

bb2:
  %p2 = call i1 @llvm.type.test(i8* null, metadata !"bar")
  br label %end

end:
  %p = phi i1 [ %p1, %bb1 ], [ %p2, %bb2 ]
  call void @llvm.assume(i1 %p)
  ret i1 %p
}

declare i1 @llvm.type.test(i8*, metadata) nounwind readnone
declare void @llvm.assume(i1)
> ra/bin/opt -module-summary foo.ll -o foo.bc && ra/bin/llvm-dis -o - foo.bc
; ModuleID = 'foo.bc'
source_filename = "foo.ll"

define i1 @f() {
  br i1 undef, label %bb1, label %bb2

bb1:                                              ; preds = %0
  %p1 = call i1 @llvm.type.test(i8* null, metadata !"foo")
  br label %end

bb2:                                              ; preds = %0
  %p2 = call i1 @llvm.type.test(i8* null, metadata !"bar")
  br label %end

end:                                              ; preds = %bb2, %bb1
  %p = phi i1 [ %p1, %bb1 ], [ %p2, %bb2 ]
  call void @llvm.assume(i1 %p)
  ret i1 %p
}

; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
declare i1 @llvm.type.test(i8*, metadata) #0

; Function Attrs: inaccessiblememonly nocallback nofree nosync nounwind willreturn
declare void @llvm.assume(i1 noundef) #1

attributes #0 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
attributes #1 = { inaccessiblememonly nocallback nofree nosync nounwind willreturn }

^0 = module: (path: "foo.bc", hash: (0, 0, 0, 0, 0))
^1 = gv: (name: "llvm.type.test") ; guid = 608142985856744218
^2 = gv: (name: "llvm.assume") ; guid = 6385187066495850096
^3 = gv: (name: "f", summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 8, typeIdInfo: (typeTests: (6699318081062747564, 16434608426314478903))))) ; guid = 14740650423002898831
^4 = blockcount: 4

This is the expected output. Note the appearance of typeTests in the summary, which means that a llvm.type.test was found that was not directly used by llvm.assume. This triggers the creation of a TTRes in the combined summary for foo and bar, as opposed to a WPDRes (you can generate the combined summary with -lowertypetests-write-summary, see e.g. llvm/test/Transforms/LowerTypeTests/export-single.ll). I'm guessing that in your case you are somehow ending up with typeTestAssumes in the summary for these type tests instead of typeTests, or that something else is preventing the TTRes from being created or causing a WPDRes to be created instead.

May 25 2022, 8:04 PM · Restricted Project, Restricted Project
tejohnson accepted D126344: [memprof] Keep and display symbol names in the RawMemProfReader..

lgtm

May 25 2022, 1:24 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D126344: [memprof] Keep and display symbol names in the RawMemProfReader..
May 25 2022, 6:38 AM · Restricted Project, Restricted Project

May 24 2022

tejohnson added a comment to D124481: [llvm][misexpect] Add tests for sample profiling.

@tejohnson I have a test case that encapsulates when SampleProfiling crashes we saw w/ misexpect a while ago.

May 24 2022, 7:38 AM · Restricted Project, Restricted Project

May 23 2022

tejohnson added inline comments to D126089: [WPD] Try harder to find assumes through phis.
May 23 2022, 2:39 PM · Restricted Project, Restricted Project

May 21 2022

tejohnson added inline comments to D126089: [WPD] Try harder to find assumes through phis.
May 21 2022, 4:34 PM · Restricted Project, Restricted Project

May 20 2022

tejohnson added inline comments to D126089: [WPD] Try harder to find assumes through phis.
May 20 2022, 2:21 PM · Restricted Project, Restricted Project

May 19 2022

tejohnson accepted D116995: [gold] Ignore bitcode from sections inside object files.

LGTM but should https://llvm.org/docs/BitCodeFormat.html#native-object-file-wrapper-format also be updated?

May 19 2022, 12:40 PM · Restricted Project, Restricted Project

May 4 2022

tejohnson committed rG655294866cf8: [memprof] Use unknown_function error type for missing functions (authored by tejohnson).
[memprof] Use unknown_function error type for missing functions
May 4 2022, 1:03 PM · Restricted Project, Restricted Project
tejohnson closed D124953: [memprof] Use unknown_function error type for missing functions.
May 4 2022, 1:03 PM · Restricted Project, Restricted Project
tejohnson requested review of D124953: [memprof] Use unknown_function error type for missing functions.
May 4 2022, 11:56 AM · Restricted Project, Restricted Project

May 2 2022

tejohnson committed rG084b65f7dc39: [memprof] Only insert dynamic shadow load when needed (authored by tejohnson).
[memprof] Only insert dynamic shadow load when needed
May 2 2022, 1:36 PM · Restricted Project, Restricted Project
tejohnson closed D124797: [memprof] Only insert dynamic shadow load when needed.
May 2 2022, 1:36 PM · Restricted Project, Restricted Project
tejohnson updated the diff for D124797: [memprof] Only insert dynamic shadow load when needed.

Address comment

May 2 2022, 12:53 PM · Restricted Project, Restricted Project
tejohnson updated the summary of D124797: [memprof] Only insert dynamic shadow load when needed.
May 2 2022, 12:43 PM · Restricted Project, Restricted Project
tejohnson requested review of D124797: [memprof] Only insert dynamic shadow load when needed.
May 2 2022, 12:41 PM · Restricted Project, Restricted Project
tejohnson committed rGa0b5af46a2a0: [memprof] Don't instrument PGO and other compiler inserted variables (authored by tejohnson).
[memprof] Don't instrument PGO and other compiler inserted variables
May 2 2022, 12:18 PM · Restricted Project, Restricted Project
tejohnson closed D124703: [memprof] Don't instrument PGO and other compiler inserted variables.
May 2 2022, 12:18 PM · Restricted Project, Restricted Project
tejohnson updated the summary of D124703: [memprof] Don't instrument PGO and other compiler inserted variables.
May 2 2022, 11:19 AM · Restricted Project, Restricted Project
tejohnson updated the diff for D124703: [memprof] Don't instrument PGO and other compiler inserted variables.

Address comments. Remove now unrelated change.

May 2 2022, 11:18 AM · Restricted Project, Restricted Project
tejohnson added inline comments to D124703: [memprof] Don't instrument PGO and other compiler inserted variables.
May 2 2022, 11:18 AM · Restricted Project, Restricted Project

Apr 29 2022

tejohnson requested review of D124703: [memprof] Don't instrument PGO and other compiler inserted variables.
Apr 29 2022, 2:58 PM · Restricted Project, Restricted Project
tejohnson committed rG6e689cbaf412: [memprof] Correct comment in test [NFC] (authored by tejohnson).
[memprof] Correct comment in test [NFC]
Apr 29 2022, 12:11 PM · Restricted Project, Restricted Project

Apr 25 2022

tejohnson accepted D124397: [Test + comment] Add a regression test to guard the 0 hot-caller threshold, and add comment near the threshold definition..

lgtm

Apr 25 2022, 11:22 AM · Restricted Project, Restricted Project
tejohnson added a comment to D124397: [Test + comment] Add a regression test to guard the 0 hot-caller threshold, and add comment near the threshold definition..

Thanks! Couple minor suggestions.

Apr 25 2022, 10:47 AM · Restricted Project, Restricted Project

Apr 22 2022

tejohnson accepted D124302: [llvm][misexpect] Avoid division by 0 when using sample profiling.

Sure, that seems fine to me. I would add a FIXME to add back the assert as well. There are a few typos in the patch description, can you fix those before submitting?

Apr 22 2022, 3:33 PM · Restricted Project, Restricted Project
tejohnson added a comment to D124302: [llvm][misexpect] Avoid division by 0 when using sample profiling.

One concern I have here, since someone reported this to me is that perhaps some of MisExpect's assumptions regarding when branch weight metadata will be attached is erroneous.

The key assumptions we make are that for frontend instrumentation that any branch weights that have been attached when LowerExpectIntrinsics runs will originate from profiling. The other key assumption we make is that when branch weights are assigned for IR instrumentation that LowerExpectIntrinsic is the only place that can add these weights before they are added by the IR or sample profiles. If that's not the case, then we need to discuss a way to address that shortcoming.

That concern doesn't change the need for this fix, but may require some more in depth discussion on discourse about how to address this in a comprehensive manner.

Apr 22 2022, 3:15 PM · Restricted Project, Restricted Project

Apr 19 2022

tejohnson added a comment to D115907: [misexpect] Re-implement MisExpect Diagnostics.

lgtm if you are waiting for a re-review. As noted in my previous comment, you have added a little bit of unnecessary checking at the lower end with your new clamp function, since it is an unsigned value and can never go below 0. But it isn't a big deal if you prefer to keep that in to make it easier to transition to std::clamp.

Apr 19 2022, 2:14 PM · Restricted Project, Restricted Project, Restricted Project

Apr 18 2022

tejohnson added inline comments to D123803: [WIP][llvm] A Unified LTO Bitcode Frontend.
Apr 18 2022, 10:05 AM · Restricted Project, Restricted Project
tejohnson added a comment to D123804: [WIP][clang][lld] A Unified LTO Bitcode Frontend.

As mentioned on the RFC thread a little bit ago, I think the split LTO handling should be separated from the unified LTO pipeline. So some of these comments may be moot if that is done.

Apr 18 2022, 10:05 AM · Restricted Project, Restricted Project

Apr 15 2022

tejohnson added inline comments to D123803: [WIP][llvm] A Unified LTO Bitcode Frontend.
Apr 15 2022, 1:46 PM · Restricted Project, Restricted Project
tejohnson added inline comments to D123803: [WIP][llvm] A Unified LTO Bitcode Frontend.
Apr 15 2022, 11:41 AM · Restricted Project, Restricted Project
tejohnson added inline comments to D123803: [WIP][llvm] A Unified LTO Bitcode Frontend.
Apr 15 2022, 11:23 AM · Restricted Project, Restricted Project

Apr 7 2022

tejohnson accepted D123094: [memprof] Deduplicate and outline frame storage in the memprof profile..

lgtm, couple minor things noted below.

Apr 7 2022, 12:53 PM · Restricted Project, Restricted Project
tejohnson committed rGb4ac84901e9b: [clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary (authored by psamolysov-intel).
[clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary
Apr 7 2022, 10:39 AM · Restricted Project, Restricted Project
tejohnson closed D123026: [clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary.
Apr 7 2022, 10:38 AM · Restricted Project, Restricted Project
tejohnson added a comment to D123026: [clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary.

Colleagues, could you help me with landing? @tejohnson has approved the patch (if I applied the suggestion as it was expected, thank you @tejohnson!)

Apr 7 2022, 8:23 AM · Restricted Project, Restricted Project
tejohnson added a comment to D123094: [memprof] Deduplicate and outline frame storage in the memprof profile..

Great! Few minor comments/questions.

Apr 7 2022, 7:59 AM · Restricted Project, Restricted Project

Apr 6 2022

tejohnson accepted D123026: [clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary.

lgtm with a couple of small changes noted below.

Apr 6 2022, 7:55 AM · Restricted Project, Restricted Project

Apr 5 2022

tejohnson committed rGced9a795fd84: [WPD] Add statistics (authored by tejohnson).
[WPD] Add statistics
Apr 5 2022, 6:48 PM · Restricted Project, Restricted Project
tejohnson closed D123152: [WPD] Add statistics.
Apr 5 2022, 6:48 PM · Restricted Project, Restricted Project
tejohnson committed rGd81b014469a5: [NFC][Bitstream] Improve the dumpability of bitstream/bitcode headers (authored by woodruffw).
[NFC][Bitstream] Improve the dumpability of bitstream/bitcode headers
Apr 5 2022, 3:11 PM · Restricted Project, Restricted Project
tejohnson closed D108438: [NFC][Bitstream] Improve the dumpability of bitstream/bitcode headers.
Apr 5 2022, 3:10 PM · Restricted Project, Restricted Project
tejohnson added a comment to D123152: [WPD] Add statistics.
In D123152#3430851, @luna wrote:

LGTM.

Only one question regarding whether the counter is intended for DevirtModule or DevirtIndex (or both)

Apr 5 2022, 3:04 PM · Restricted Project, Restricted Project
tejohnson requested review of D123152: [WPD] Add statistics.
Apr 5 2022, 12:43 PM · Restricted Project, Restricted Project
tejohnson added a comment to D123026: [clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary.

I'd prefer to keep the setting of the module flags inline here. The main reason is that the EnableSplitLTOUnit module flag is also set inline for the ThinLTO case just above, and it is easier to see what is going on if they are near each other. Suggest making the new emitLTOSummary just about checking the conditions for emitting the summary.

Apr 5 2022, 8:32 AM · Restricted Project, Restricted Project

Apr 4 2022

tejohnson added inline comments to D115907: [misexpect] Re-implement MisExpect Diagnostics.
Apr 4 2022, 9:55 PM · Restricted Project, Restricted Project, Restricted Project
tejohnson accepted D120230: [SelectOpti][1/5] Setup new select-optimize pass.

One comment about the patch is that it would be good to remove the llvm_unreachable, and test for the pass in one of the pass ordering tests. E.g. llvm/test/CodeGen/X86/opt-pipeline.ll (there are similar ones for other archs too).

In this patch series the pass is disabled by default and I was actually planning on having a separate follow-up patch (D121547) where I will enable by default this pass for x86 instr-PGO. In the D121547 patch I had to change the X86/opt-pipeline.ll and you can see more clearly the placement of this pass (almost just before the CodeGenPrepare pass). If it is preferable I can move these changes in this patch.

I see, ok it seems reasonable to leave the pass pipeline testing until then. But generally it is good to have tests with each patch - another option is to merge this with the next patch which I assume has part of the implementation in it, and add an opt based test with that. Oh, I see that patches 2 and 3 don't have a test, only patch 4. IMO if at all possible it is better to split up the patches into pieces that can each be tested.

The reason that all the tests are in the 4th patch is that this patch involves the actual transformation (converts selects to branches for the cases that the conversion was deemed profitable based on the profitability heuristics of patches 2 and 3). The 2nd patch has the base (non-loop) heuristics and the 3rd patch has the loop-level heuristics. Patches 2 and 3 do not change the IR.

I agree though that it is better to split it up in a way that allows testing of each patch. So, I will re-organize the patches to enable more per-patch testing. I will move the base of the actual transformation of the code in a 2nd patch (and some testing by temporarily assuming that all selects should be converted), then the 3rd patch will be the base heuristics and testing, 4th patch loop heuristics and testing, and a 5th patch that optimizes the transformation (maximizes the sinking of one-use slices in the true/false blocks and interleaving of slices).

As discussed, re-organized the subsequent patches to allow for per-patch testing.

@tejohnson does the re-organization of subsequent patches to allow for per-patch testing look okay to you? Is there anything else to note for this first patch? Apart from the decision of where to place the new pass, the rest of the code is mostly boilerplate.

Apr 4 2022, 9:41 PM · Restricted Project, Restricted Project

Mar 29 2022

tejohnson added a comment to D108438: [NFC][Bitstream] Improve the dumpability of bitstream/bitcode headers.

Gentle ping for re-app/merge.

Mar 29 2022, 8:53 PM · Restricted Project, Restricted Project

Mar 28 2022

tejohnson added a comment to D115907: [misexpect] Re-implement MisExpect Diagnostics.

@tejohnson thanks for pointing me to the document. I knew it had something to do w/ CC1 but missed that this was well documented.

Mar 28 2022, 4:27 PM · Restricted Project, Restricted Project, Restricted Project
tejohnson added inline comments to D115907: [misexpect] Re-implement MisExpect Diagnostics.
Mar 28 2022, 9:57 AM · Restricted Project, Restricted Project, Restricted Project
tejohnson accepted D122587: [clang][NFC] Extract the EmitAssemblyHelper::TargetTriple member.

lgtm

Mar 28 2022, 8:56 AM · Restricted Project, Restricted Project

Mar 27 2022

tejohnson accepted D122473: Add new explanation for some shortcomings(WPD, CFI) for lexicon.

lgtm

Mar 27 2022, 8:27 PM · Restricted Project, Restricted Project

Mar 25 2022

tejohnson added a comment to D122473: Add new explanation for some shortcomings(WPD, CFI) for lexicon.

Thanks for adding these. One suggestion/question below.

Mar 25 2022, 8:08 AM · Restricted Project, Restricted Project

Mar 22 2022

tejohnson accepted D122260: [memprof] Initialize MemInfoBlock data..

lgtm

Mar 22 2022, 2:32 PM · Restricted Project, Restricted Project, Restricted Project
tejohnson added inline comments to D122260: [memprof] Initialize MemInfoBlock data..
Mar 22 2022, 2:06 PM · Restricted Project, Restricted Project, Restricted Project

Mar 21 2022

tejohnson accepted D121179: [memprof] Store callsite metadata with memprof records..

lgtm

Mar 21 2022, 10:55 AM · Restricted Project, Restricted Project
tejohnson accepted D122033: [DebugInfo][NFC] Add a comment on the ordering of DILineInfo frames..

(but lgtm otherwise)

Mar 21 2022, 8:48 AM · Restricted Project, Restricted Project
tejohnson added inline comments to D122033: [DebugInfo][NFC] Add a comment on the ordering of DILineInfo frames..
Mar 21 2022, 8:44 AM · Restricted Project, Restricted Project

Mar 17 2022

tejohnson accepted D115907: [misexpect] Re-implement MisExpect Diagnostics.

Just some minor cleanup before submitting as noted below.

Mar 17 2022, 3:16 PM · Restricted Project, Restricted Project, Restricted Project
tejohnson added inline comments to D121179: [memprof] Store callsite metadata with memprof records..
Mar 17 2022, 2:07 PM · Restricted Project, Restricted Project
tejohnson accepted D121888: [LTO][ELF] Require asserts for --stats-file= tests..

Sorry for the patch abuse, the pre-merge build bot didn't catch the failures, like https://lab.llvm.org/buildbot/#/builders/124/builds/3948 and https://lab.llvm.org/buildbot/#/builders/67/builds/6214. Hopefully, this patch will fix it.

Mar 17 2022, 8:01 AM · Restricted Project, lld, Restricted Project

Mar 16 2022

tejohnson added a comment to D121809: [LTO][ELF] Add --stats-file= option..

Sorry to interrupt, seems this patch caused the build failure, see https://lab.llvm.org/buildbot/#/builders/124/builds/3944. I don't have any clue why this failed, Did I miss something for the test file? Like ; REQUIRES: x86 is indispensable?

Mar 16 2022, 11:44 PM · Restricted Project, lld, Restricted Project
tejohnson accepted D121809: [LTO][ELF] Add --stats-file= option..

lgtm

Mar 16 2022, 8:47 PM · Restricted Project, lld, Restricted Project
tejohnson accepted D121830: [memprof] Update the frame is inline logic and unittests..

Unfortunately the ordering is not documented on the DIInliningInfo class. And confusingly, one piece of code sets an Inlined flag by the check "I > 0" (https://github.com/llvm/llvm-project/blob/49c048add4c980936fc2918838288ae2d795587d/llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp#L198).
However, if I look at how that flag is used, it is used to print "(inlined by)" *before* the current frame's function name, so it seems to apply to the I-1 frame, which actually means that all but the last frame are inlined - consistent with the change you have made here. Consider making a separate NFC change to add a comment to DIInliningInfo?

Mar 16 2022, 4:19 PM · Restricted Project, Restricted Project
tejohnson accepted D121809: [LTO][ELF] Add --stats-file= option..

lgtm. Few minor comments to address below before submitting.

Mar 16 2022, 8:51 AM · Restricted Project, lld, Restricted Project

Mar 15 2022

tejohnson added inline comments to D115907: [misexpect] Re-implement MisExpect Diagnostics.
Mar 15 2022, 11:14 PM · Restricted Project, Restricted Project, Restricted Project