Page MenuHomePhabricator

philip.pfaffe (Philip Pfaffe)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 21 2015, 2:43 AM (212 w, 5 d)

Recent Activity

Fri, May 10

philip.pfaffe accepted D61709: [NewPM] Port HWASan and Kernel HWASan.

Nit aside, looks good!

Fri, May 10, 12:22 PM · Restricted Project, Restricted Project

Wed, May 8

philip.pfaffe accepted D61664: [NewPM] Setup Passes for KASan and KMSan.

LGTM.

Wed, May 8, 12:11 AM · Restricted Project, Restricted Project

Apr 19 2019

philip.pfaffe added a comment to D60870: POC. [PassBuilder] introduce a separate pass-pipeline parsing API.

To clarify that: Currently clients are offered two APIs to populate a PassManager: One that consumes PipelineElements and one that consumes a string. When you have a PipelineElement in hand, you could use both, because it's mostly just a StringRef! I'd like to avoid that ambiguity.

Apr 19 2019, 3:09 AM · Restricted Project
philip.pfaffe added a comment to D60870: POC. [PassBuilder] introduce a separate pass-pipeline parsing API.

IMO it makes sense to separate parsing of the pipeline text and the pipeline tree more. Parsing the pipeline string into the tree of pipeline elements is an operation that happens once and as the very first thing, after which the string is not needed anymore. The PassBuilder could do that part, and then call the PipelineParser to further parse the tree. Otherwise this looks great!

Apr 19 2019, 3:02 AM · Restricted Project

Apr 17 2019

philip.pfaffe added a comment to D60814: [PassBuilder] promote pass-pipeline parsing API to public.

Yeah, the array-ref API here is already pretty leaky. It'd be really nice if we could only give the callbacks the necessary APIs here, but not sure its worth building that abstraction. I think the way I'd do it is create a parser type that is only built by the PassBuilder and is passed into the callbacks. It would then have these APIs, etc. It'd also be used internally to implement the main parsing API. Thoughts?

Apr 17 2019, 8:18 AM · Restricted Project
philip.pfaffe added a comment to D60814: [PassBuilder] promote pass-pipeline parsing API to public.

What is your specific usecase for this? You have a custom PassManager which can contain, e.g., a function pass pipeline?

Apr 17 2019, 1:46 AM · Restricted Project

Mar 27 2019

philip.pfaffe accepted D59869: [NewPM] Fix a nasty bug with analysis invalidation in the new PM..

Okay. Any plans to improve it? On the other hand, I'm not sure about the impact. We'll probably have to get some profile data first.

Mar 27 2019, 12:59 PM · Restricted Project
philip.pfaffe added a comment to D59869: [NewPM] Fix a nasty bug with analysis invalidation in the new PM..

This makes complete sense to me. There is currently no way to invalidate an analysis for a specific IRUnit instance only, right? So not preserving an analysis cross-SCC will invalidate it for all?

Mar 27 2019, 7:46 AM · Restricted Project

Mar 22 2019

philip.pfaffe accepted D59416: [Legacy][TimePasses] allow -time-passes reporting into a custom stream.

On final language nit, otherwise LGTM!

Mar 22 2019, 5:42 AM · Restricted Project

Mar 20 2019

philip.pfaffe added inline comments to D59416: [Legacy][TimePasses] allow -time-passes reporting into a custom stream.
Mar 20 2019, 10:48 AM · Restricted Project
philip.pfaffe added inline comments to D59416: [Legacy][TimePasses] allow -time-passes reporting into a custom stream.
Mar 20 2019, 4:53 AM · Restricted Project
philip.pfaffe added a comment to D59416: [Legacy][TimePasses] allow -time-passes reporting into a custom stream.

Just two inline nits.

Mar 20 2019, 3:57 AM · Restricted Project

Mar 15 2019

philip.pfaffe accepted D59366: [NewPM][TimePasses] allow -time-passes reporting into a custom stream.

Looks good!

Mar 15 2019, 11:48 AM · Restricted Project
philip.pfaffe added a comment to D59366: [NewPM][TimePasses] allow -time-passes reporting into a custom stream.

Couple of nits inline, otherwise looks great!

Mar 15 2019, 5:13 AM · Restricted Project

Mar 14 2019

philip.pfaffe added a comment to D59366: [NewPM][TimePasses] allow -time-passes reporting into a custom stream.

CreateInfoOutputFile creates a new stream and unless we retake the ownership it will be destroyed.

True, but that's what it is doing right now. Do you see reason to change that?

Mar 14 2019, 9:13 AM · Restricted Project
philip.pfaffe added a comment to D59366: [NewPM][TimePasses] allow -time-passes reporting into a custom stream.

Why not default to CreateInfoOutputFile inside of TimePassesHandler if OutputStream is unset? And then have a setStream(raw_ostream&) method to change it.

Mar 14 2019, 8:59 AM · Restricted Project

Mar 12 2019

philip.pfaffe accepted D59258: [Docs] Add a note about legacy FunctionPassManager to the LLVM tutorial..

LGTM. Did you verify that there's no conflicting info in the first three chapters of the tutorial?

Mar 12 2019, 8:33 AM · Restricted Project

Feb 27 2019

philip.pfaffe added inline comments to D58406: Fix IR/Analysis layering issue in OptBisect.
Feb 27 2019, 4:30 AM · Restricted Project
philip.pfaffe added a comment to D58572: [Support] allow -stats/-time-passes reporting into a custom stream.

Do you suggest to get rid of the current scheme where we have a single output file for all kinds of "info data"?

No I don't, that should certainly be the default.

Feb 27 2019, 2:07 AM · Restricted Project
philip.pfaffe added a comment to D58572: [Support] allow -stats/-time-passes reporting into a custom stream.

This change is focused purely on the stream that LLVM uses to report the data being collected.
No matter how many TimePassesHandlers you create (and yes, we do create TimePassesHandler one per thread-per-pipeline) they all emit into the same CreateInfoOutputFile() - either stderr or a single file.
I'm trying to solve a problem of a badly interleaved output, thats all.

You could set the output directly on the handler.

Feb 27 2019, 1:23 AM · Restricted Project

Feb 25 2019

philip.pfaffe added a comment to D58572: [Support] allow -stats/-time-passes reporting into a custom stream.

I don't think this is sufficient or goes into the right direction to support parallel info collection and output.

Feb 25 2019, 4:10 AM · Restricted Project

Feb 20 2019

philip.pfaffe added a comment to D58406: Fix IR/Analysis layering issue in OptBisect.

Keep in mind that for the new PM we're talking about an API that's much more powerful, because it's designed for arbitrary hooks.

Feb 20 2019, 9:08 AM · Restricted Project

Feb 19 2019

philip.pfaffe abandoned D57793: [NewPM][MSan] Add sanitizer at O0.
Feb 19 2019, 9:20 PM · Restricted Project

Feb 13 2019

philip.pfaffe accepted D56470: [NewPM] Second attempt at porting ASan.

Tiny final nit, otherwise looks great!

Feb 13 2019, 12:31 PM · Restricted Project, Restricted Project

Feb 11 2019

philip.pfaffe added a comment to D57265: [PM/CC1] Add -f[no-]split-cold-code CC1 options to toggle splitting.

I'd prefer not adding this kind of state to PassBuilder. SplitColdCode is soemthing that refers to the construction of one particular pipeline, not to pipeline-building in general. It should be an argument passed down through the build*Pipeline calls.

Feb 11 2019, 3:16 AM · Restricted Project

Feb 9 2019

philip.pfaffe added a comment to D56470: [NewPM] Second attempt at porting ASan.

It's still missing the O0 part. I forgot adding it for msan and tsan, but we should do asan correctly from the start.

Feb 9 2019, 3:54 AM · Restricted Project, Restricted Project

Feb 5 2019

philip.pfaffe created D57793: [NewPM][MSan] Add sanitizer at O0.
Feb 5 2019, 3:18 PM · Restricted Project

Feb 4 2019

philip.pfaffe committed rG0ee6a933cec4: [NewPM][MSan] Add Options Handling (authored by philip.pfaffe).
[NewPM][MSan] Add Options Handling
Feb 4 2019, 1:03 PM
philip.pfaffe requested changes to D56470: [NewPM] Second attempt at porting ASan.

Sorry, there's something I missed, comment inline.

Feb 4 2019, 12:59 PM · Restricted Project, Restricted Project
philip.pfaffe updated the diff for D57640: [NewPM][MSan] Add Options Handling.

Add the default argument back.

Feb 4 2019, 12:45 PM · Restricted Project, Restricted Project

Feb 3 2019

philip.pfaffe committed rG9438585fe457: [DA][NewPM] Handle transitive dependencies in the new-pm version of DA (authored by philip.pfaffe).
[DA][NewPM] Handle transitive dependencies in the new-pm version of DA
Feb 3 2019, 4:28 AM

Feb 2 2019

philip.pfaffe accepted D56470: [NewPM] Second attempt at porting ASan.

Great! Just three more nits. Feel free to land after adressing them!

Feb 2 2019, 3:31 PM · Restricted Project, Restricted Project
philip.pfaffe added a comment to D56935: [NewPM] Add support for new-PM plugins to clang.

Landed it for you in r352972. Thanks!

Feb 2 2019, 3:20 PM · Restricted Project
philip.pfaffe created D57640: [NewPM][MSan] Add Options Handling.
Feb 2 2019, 12:49 PM · Restricted Project, Restricted Project

Jan 30 2019

philip.pfaffe added a comment to D56470: [NewPM] Second attempt at porting ASan.

This is starting to look great! Minor nits inline.

Jan 30 2019, 11:41 AM · Restricted Project, Restricted Project

Jan 25 2019

philip.pfaffe accepted D56935: [NewPM] Add support for new-PM plugins to clang.

It would be good to check, since the bots won't! Otherwise this looks good.

Jan 25 2019, 12:57 AM · Restricted Project
philip.pfaffe added inline comments to D56470: [NewPM] Second attempt at porting ASan.
Jan 25 2019, 12:49 AM · Restricted Project, Restricted Project

Jan 23 2019

philip.pfaffe added a comment to D56935: [NewPM] Add support for new-PM plugins to clang.

I'm not sure what the current state of plugins on windows is. They were broken and disabled last time I worked on this, but that might've changed in the meantime! Worth checking.

Jan 23 2019, 3:32 AM · Restricted Project
philip.pfaffe added a comment to D56470: [NewPM] Second attempt at porting ASan.

One thing I missed.

Jan 23 2019, 3:19 AM · Restricted Project, Restricted Project
philip.pfaffe added a comment to D56470: [NewPM] Second attempt at porting ASan.

Nice, we're getting somewhere! A lot of comments inline. Most importantly, your analysis is a function analysis, which it mustn't be, otherwise there's no gain from it at all.

Jan 23 2019, 3:15 AM · Restricted Project, Restricted Project

Jan 22 2019

philip.pfaffe added a comment to D56935: [NewPM] Add support for new-PM plugins to clang.

This generally looks sane. What will happen on windows though? Will it silently fail?

Jan 22 2019, 12:13 PM · Restricted Project

Jan 18 2019

philip.pfaffe added a comment to D56470: [NewPM] Second attempt at porting ASan.

I ran a benchmark using asan on a synthetic testcase made up of 10k functions and 10k globals, and it's clearly visible that GlobalsMD causes the performance problem. This is because during initialization it looks at all the globals registered in llvm.asan.globals, which is used to mark the globals coming from the frontend which are supposed to be instrumented. The function pass, however, doesn't use most of this, and accesses GlobalsMD only within AddressSanitizer::GlobalIsLinkerInitialized to check whether a global is dynamically initialized. This means we can fix this in two ways:

Jan 18 2019, 9:01 AM · Restricted Project, Restricted Project

Jan 17 2019

philip.pfaffe created D56846: [NewPM] Catch sanitizers passed to -fsanitize that haven't been implemented.
Jan 17 2019, 4:39 AM
philip.pfaffe created D56831: [NewPM] Add -fsanitize={memory,thread} handling to clang.
Jan 17 2019, 1:54 AM

Jan 16 2019

philip.pfaffe added a comment to D56433: [NewPM] Port tsan.

@delcypher Thanks for pointing it out! Just landed a revised version in rL351314.

Jan 16 2019, 3:20 AM
philip.pfaffe updated the diff for D56734: [MSan] Apply the ctor creation scheme of TSan.

Address comments and update two missing testcases.

Jan 16 2019, 1:18 AM

Jan 15 2019

philip.pfaffe added a parent revision for D56734: [MSan] Apply the ctor creation scheme of TSan: D56538: [NewPM][TSan] Reiterate the TSan port.
Jan 15 2019, 10:57 AM
philip.pfaffe added a child revision for D56538: [NewPM][TSan] Reiterate the TSan port: D56734: [MSan] Apply the ctor creation scheme of TSan.
Jan 15 2019, 10:57 AM
philip.pfaffe created D56734: [MSan] Apply the ctor creation scheme of TSan.
Jan 15 2019, 10:56 AM
philip.pfaffe updated the diff for D56538: [NewPM][TSan] Reiterate the TSan port.

Execute appendtoGlobalCtors lazily as well. Previously the inserted ctor was
still added multiple times to the global ctors list. Doing this lazily requires
knowing when the ctor is actually created, so I added a callback to handle the
insertion.

Jan 15 2019, 10:51 AM

Jan 11 2019

philip.pfaffe updated the diff for D56538: [NewPM][TSan] Reiterate the TSan port.

Insert the constructor and init functions only once per module.

Jan 11 2019, 3:44 AM
philip.pfaffe added inline comments to D56538: [NewPM][TSan] Reiterate the TSan port.
Jan 11 2019, 2:08 AM
philip.pfaffe updated the diff for D56538: [NewPM][TSan] Reiterate the TSan port.

Fix a comment.

Jan 11 2019, 2:04 AM

Jan 10 2019

philip.pfaffe updated the diff for D56538: [NewPM][TSan] Reiterate the TSan port.

Add missing license to TSan header.

Jan 10 2019, 4:20 AM
philip.pfaffe created D56538: [NewPM][TSan] Reiterate the TSan port.
Jan 10 2019, 4:16 AM
philip.pfaffe accepted D53895: [LoopUnroll] add parsing for unroll parameters in -passes pipeline.

Looks good to me, thanks!

Jan 10 2019, 12:44 AM

Jan 8 2019

philip.pfaffe accepted D56452: Fix go bindings for r350647: missed a function rename.

Thanks! Sorry that I missed this.

Jan 8 2019, 1:48 PM
philip.pfaffe updated the diff for D56381: [DA][NewPM] Handle transitive dependencies in the new-pm version of DA.

Rebase (and drop changes from D56386 that snuck in).

Jan 8 2019, 6:25 AM · Restricted Project
philip.pfaffe created D56433: [NewPM] Port tsan.
Jan 8 2019, 4:40 AM

Jan 7 2019

philip.pfaffe updated the diff for D56381: [DA][NewPM] Handle transitive dependencies in the new-pm version of DA.

Rebase onto D56386 and add testcase for DA invalidation

Jan 7 2019, 4:32 AM · Restricted Project
philip.pfaffe created D56386: [DA][NewPM] Add a printerpass and port the testsuite.
Jan 7 2019, 4:30 AM
philip.pfaffe created D56381: [DA][NewPM] Handle transitive dependencies in the new-pm version of DA.
Jan 7 2019, 3:17 AM · Restricted Project

Jan 5 2019

philip.pfaffe added a comment to D53895: [LoopUnroll] add parsing for unroll parameters in -passes pipeline.

Cool! Down to bikeshed comments :)

Jan 5 2019, 8:19 AM

Jan 3 2019

philip.pfaffe added a comment to D54337: [ASan] Make AddressSanitizer a ModulePass.

I haven't gotten back to this yet, but will do so after looking into what may be required for also porting SafeStack and ShadowCallStack to new PM.

If I recall correctly, it was pointed out on the SafeStack thread that, since SafeStack is part of CodeGen, porting it is not possible right now.

Jan 3 2019, 10:50 AM · Restricted Project, Restricted Project
philip.pfaffe added inline comments to D53895: [LoopUnroll] add parsing for unroll parameters in -passes pipeline.
Jan 3 2019, 7:46 AM
philip.pfaffe added a comment to D54337: [ASan] Make AddressSanitizer a ModulePass.

@leonardchan Are you making any progress here?

Jan 3 2019, 6:16 AM · Restricted Project, Restricted Project
philip.pfaffe abandoned D54394: [WIP][NewPM] Port msan.

Superseded by D55647.

Jan 3 2019, 6:03 AM
philip.pfaffe added inline comments to D53895: [LoopUnroll] add parsing for unroll parameters in -passes pipeline.
Jan 3 2019, 3:09 AM

Jan 2 2019

philip.pfaffe updated the diff for D55647: [NewPM] Port Msan: Second alternative approach without any module level initalization.

Addressed comments and rebased on top of rL350219.

Jan 2 2019, 10:12 AM
philip.pfaffe updated the diff for D56130: Extend Module::getOrInsertGlobal to also take the default arguments accepted by the GlobalVariable constructor..

Follow @chandlerc's proposal for a much more narrow API.

Jan 2 2019, 3:27 AM

Dec 29 2018

philip.pfaffe added a comment to D56130: Extend Module::getOrInsertGlobal to also take the default arguments accepted by the GlobalVariable constructor..

That solves the default case, but I feel like the new three-argument case is still not much better than getGlobalVariable and an if.

Dec 29 2018, 3:32 PM
philip.pfaffe added a comment to D56130: Extend Module::getOrInsertGlobal to also take the default arguments accepted by the GlobalVariable constructor..

I get the idea of trying to separate query and creation arguments to make their meaning explicit. But how do you expect this to be used? The default call (i.e., the one everyone is using right now), would then look like

M.getOrInsertGlobal("Global", Ctx.getIntTy(), [] { return new GlobalVariable("Global", Ctx.getIntTy()); });

That's super verbose, and to me at least it's much harder to read then just calling Module::getGlobalVariable in an if.

Dec 29 2018, 2:47 PM

Dec 28 2018

philip.pfaffe added a comment to D56130: Extend Module::getOrInsertGlobal to also take the default arguments accepted by the GlobalVariable constructor..

The extra arguments are not filters. The idea here is to basicly be a mostly-drop-in replacement for calling the GlobalVariable constructor.

Dec 28 2018, 9:07 AM
philip.pfaffe created D56130: Extend Module::getOrInsertGlobal to also take the default arguments accepted by the GlobalVariable constructor..
Dec 28 2018, 5:34 AM

Dec 27 2018

philip.pfaffe added inline comments to D55647: [NewPM] Port Msan: Second alternative approach without any module level initalization.
Dec 27 2018, 5:53 AM

Dec 24 2018

philip.pfaffe added a comment to D55605: [NewPM] Port Msan: Alternative approach using a module to create the global constructor.

A couple of nits.
I also think the newly added classes should be marked as such, not as structs, but YMMV.

Dec 24 2018, 5:37 AM
philip.pfaffe added a comment to D55605: [NewPM] Port Msan: Alternative approach using a module to create the global constructor.

Absolutely :)

Dec 24 2018, 4:33 AM
philip.pfaffe updated the diff for D55647: [NewPM] Port Msan: Second alternative approach without any module level initalization.

Address comments:

  • Move init creator function next to the ctor function
  • Use Module::getOrInsertGlobal instead of my own version, but modify it to fit sanitizer neads
Dec 24 2018, 4:32 AM

Dec 23 2018

philip.pfaffe abandoned D55605: [NewPM] Port Msan: Alternative approach using a module to create the global constructor.

Abandoning in favor of D55605.

Dec 23 2018, 4:20 AM

Dec 20 2018

philip.pfaffe accepted D55278: [NewPM] -print-module-scope -print-after now prints module even after invalidated Loop/SCC.

Thanks, LGTM!

Dec 20 2018, 6:18 AM
philip.pfaffe added inline comments to D55278: [NewPM] -print-module-scope -print-after now prints module even after invalidated Loop/SCC.
Dec 20 2018, 4:37 AM
philip.pfaffe accepted D51748: cmake: Remove add_llvm_loadable_module().

This looks mostly like a mechanical change now, so LGTM!

Dec 20 2018, 2:28 AM

Dec 17 2018

philip.pfaffe added inline comments to D55278: [NewPM] -print-module-scope -print-after now prints module even after invalidated Loop/SCC.
Dec 17 2018, 1:40 PM
philip.pfaffe added a comment to D55278: [NewPM] -print-module-scope -print-after now prints module even after invalidated Loop/SCC.

This generally looks fine, some code comments inline.

Dec 17 2018, 1:30 PM

Dec 14 2018

philip.pfaffe added a comment to D54337: [ASan] Make AddressSanitizer a ModulePass.

It depends on what you mean by task. There is no clean way for depending on changes between IR units. I.e. you can't express the requirement that two passes have to run in a specific order. But that's okay, implementing such requirements would incure insane complexity. Fortunately, if I recall correctly Asan oesn't need that! Correct me if I'm wrong.

Dec 14 2018, 2:38 AM · Restricted Project, Restricted Project
philip.pfaffe added a comment to D55605: [NewPM] Port Msan: Alternative approach using a module to create the global constructor.

I dropped MSanOptions over in D55647 (or didn't need it there), which will likely supersede this change here.

Dec 14 2018, 2:12 AM
philip.pfaffe added inline comments to D55647: [NewPM] Port Msan: Second alternative approach without any module level initalization.
Dec 14 2018, 2:11 AM

Dec 13 2018

philip.pfaffe created D55647: [NewPM] Port Msan: Second alternative approach without any module level initalization.
Dec 13 2018, 3:18 AM

Dec 12 2018

philip.pfaffe updated the summary of D55605: [NewPM] Port Msan: Alternative approach using a module to create the global constructor.
Dec 12 2018, 10:04 AM
philip.pfaffe created D55605: [NewPM] Port Msan: Alternative approach using a module to create the global constructor.
Dec 12 2018, 9:33 AM

Dec 11 2018

philip.pfaffe added a comment to D54740: [NewPM] fixing asserts on deleted loop in -print-after-all.

Not really. If you say have a Loop* pointer, which is freed and then another Loop allocated happens to be just at that pointer
then you dont have a way to identify whether it is the old Loop or the new one.
Presumably that does not happen now...

Dec 11 2018, 4:44 AM

Dec 10 2018

philip.pfaffe added a comment to D54740: [NewPM] fixing asserts on deleted loop in -print-after-all.

The invalid handle isn't good for accessing anything, but it can still identify a unit.

Dec 10 2018, 1:59 PM
philip.pfaffe added a comment to D54740: [NewPM] fixing asserts on deleted loop in -print-after-all.

The IRUnit is invalid, but the handle still exists, right?

Dec 10 2018, 1:29 PM
philip.pfaffe accepted D54740: [NewPM] fixing asserts on deleted loop in -print-after-all.
Dec 10 2018, 7:05 AM
philip.pfaffe added a comment to D54740: [NewPM] fixing asserts on deleted loop in -print-after-all.

Thanks, this does LGTM!

Dec 10 2018, 5:36 AM

Dec 7 2018

philip.pfaffe added inline comments to D54740: [NewPM] fixing asserts on deleted loop in -print-after-all.
Dec 7 2018, 6:39 AM

Dec 6 2018

philip.pfaffe added a comment to D55278: [NewPM] -print-module-scope -print-after now prints module even after invalidated Loop/SCC.

What exactly does this change solve?

Dec 6 2018, 9:42 AM

Dec 5 2018

philip.pfaffe added a comment to D54394: [WIP][NewPM] Port msan.

I think I'm missing some context here.
What is the problem you are trying to solve?

msan as most sanitizers creates a new function to initialize itself, which function passes cannot do.

Technically they can, at least now. Are there any upcoming changes to that?

Dec 5 2018, 6:44 AM
philip.pfaffe added a comment to D54394: [WIP][NewPM] Port msan.
  • Make a free function for creating the global constructor function
  • The opt tool calls this function when passing a command line option

What is the benefit of having free function compared to having a module-level pass that does the same?

Having a pass introduces dependencies between passes, which I find unsetteling. I.e. when you use the asan pass, you _must_ also use the asan-init pass, or otherwise you end up with IR that you can't compile without linker errors. That being said, the same is of course true when using the command line argument approach, so asan-init might not be the worst at the end of the day.

Dec 5 2018, 6:43 AM

Dec 3 2018

philip.pfaffe added inline comments to D54740: [NewPM] fixing asserts on deleted loop in -print-after-all.
Dec 3 2018, 4:43 AM