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 (196 w, 1 d)

Recent Activity

Today

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.

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

One thing I missed.

Wed, Jan 23, 3:19 AM
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.

Wed, Jan 23, 3:15 AM

Yesterday

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?

Tue, Jan 22, 12:13 PM · Restricted Project

Fri, Jan 18

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:

Fri, Jan 18, 9:01 AM

Thu, Jan 17

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

Wed, Jan 16

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

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

Wed, Jan 16, 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.

Wed, Jan 16, 1:18 AM

Tue, Jan 15

philip.pfaffe added a parent revision for D56734: [MSan] Apply the ctor creation scheme of TSan: D56538: [NewPM][TSan] Reiterate the TSan port.
Tue, Jan 15, 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.
Tue, Jan 15, 10:57 AM
philip.pfaffe created D56734: [MSan] Apply the ctor creation scheme of TSan.
Tue, Jan 15, 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.

Tue, Jan 15, 10:51 AM

Fri, Jan 11

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

Insert the constructor and init functions only once per module.

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

Fix a comment.

Fri, Jan 11, 2:04 AM

Thu, Jan 10

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

Add missing license to TSan header.

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

Looks good to me, thanks!

Thu, Jan 10, 12:44 AM

Tue, Jan 8

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

Thanks! Sorry that I missed this.

Tue, Jan 8, 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).

Tue, Jan 8, 6:25 AM
philip.pfaffe created D56433: [NewPM] Port tsan.
Tue, Jan 8, 4:40 AM

Mon, Jan 7

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

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

Sat, Jan 5

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

Cool! Down to bikeshed comments :)

Sat, Jan 5, 8:19 AM

Thu, Jan 3

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.

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

@leonardchan Are you making any progress here?

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

Superseded by D55647.

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

Wed, Jan 2

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.

Wed, Jan 2, 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.

Wed, Jan 2, 3:27 AM

Sat, Dec 29

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.

Sat, Dec 29, 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.

Sat, Dec 29, 2:47 PM

Fri, Dec 28

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.

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

Thu, Dec 27

philip.pfaffe added inline comments to D55647: [NewPM] Port Msan: Second alternative approach without any module level initalization.
Thu, Dec 27, 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
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

Nov 21 2018

philip.pfaffe added a comment to D54794: [NewPM] -print-before/-print-after support for new pass manager.

Is the goal here anything beyond makeing -print-after/before work in the newpm?

Nov 21 2018, 2:04 PM

Nov 16 2018

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

Sorry for the delayed response. So the main problem is that AddressSantizer needs to perform some initialization involving reading stuff from global metadata and getting some target specific information. More specifically, just these 6 things which are all initialized in doInitialization:

GlobalsMD.init(M);
C = &(M.getContext());
LongSize = M.getDataLayout().getPointerSizeInBits();
IntptrTy = Type::getIntNTy(*C, LongSize);
TargetTriple = Triple(M.getTargetTriple());
Mapping = getShadowMapping(TargetTriple, LongSize, CompileKernel);

If asan doesn't modify the module for this ('this' meaning the Mapping and the GlobalsMD here), than this is an excellent candidate for an analysis.

Nov 16 2018, 1:17 AM · Restricted Project

Nov 13 2018

philip.pfaffe retitled D54394: [WIP][NewPM] Port msan from [NewPM] Port msan to [WIP][NewPM] Port msan.
Nov 13 2018, 11:08 AM

Nov 11 2018

philip.pfaffe created D54394: [WIP][NewPM] Port msan.
Nov 11 2018, 10:28 AM

Nov 10 2018

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

Hmm... can you point me where function definitions are being added in doInitialization, I just dont see it (frankly I'm not familiar with this code at all).

I might have been wrong here, I assumed the Asan function pass did this,too, because all sanitizers I've looked at do it. But it's possible that only the Module variant does it.

Nov 10 2018, 4:02 AM · Restricted Project
philip.pfaffe added a comment to D54337: [ASan] Make AddressSanitizer a ModulePass.

The problem is not speed but correctness. The initialization adds function definitions, which function passes can't do. So we need to initialize out-of-band, either with a separate module pass, or some function outside of the pass pipeline. I strongly prefer the latter, but it's not entirely clear to me how this function should be called. It's obviousl when running this through clang, but what about opt?

Nov 10 2018, 3:01 AM · Restricted Project
philip.pfaffe added a comment to D54374: [newpm] Add an OptimizerLast EP.

A key difference between this and legacy is at O0, this EP won't be triggered.

Nov 10 2018, 2:25 AM
philip.pfaffe added a comment to D54337: [ASan] Make AddressSanitizer a ModulePass.

Making this a module pass will cost us all the nice chaining and cache locality we get from a function pass. That will make the already slow instrumentation even more expensive. Do you see a way to keep this as a function pass?

Nov 10 2018, 1:18 AM · Restricted Project
philip.pfaffe created D54374: [newpm] Add an OptimizerLast EP.
Nov 10 2018, 1:08 AM

Nov 6 2018

philip.pfaffe added a comment to D53376: Fix generation of exported targets in build directory.

Commited!

Nov 6 2018, 7:21 AM

Nov 2 2018

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

What is the advantage of this over using command line args?

Nov 2 2018, 9:54 AM

Oct 30 2018

philip.pfaffe abandoned D49486: [cfe][CMake] Export the clang resource directory.

It looks like running clang will be good enough. I'm closing this for now!

Oct 30 2018, 4:50 AM

Oct 23 2018

philip.pfaffe accepted D53376: Fix generation of exported targets in build directory.

LGTM, thanks!

Oct 23 2018, 6:15 AM

Oct 22 2018

philip.pfaffe added a comment to D53376: Fix generation of exported targets in build directory.

The fix looks alright, but did you test it with CMAKE_BUILD_TYPE set though? It's hard to parse with the naked eye, but it looks like you're missing at least a $ there.

Oct 22 2018, 3:40 AM

Oct 17 2018

philip.pfaffe accepted D52814: [PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with -fsanitize=address .

LGTM, thank you!

Oct 17 2018, 4:44 AM · Restricted Project, Restricted Project

Oct 15 2018

philip.pfaffe added a comment to D53246: [NewPM] teach -passes= to emit meaningful error messages.

Thanks!

Oct 15 2018, 7:07 AM
philip.pfaffe accepted D53246: [NewPM] teach -passes= to emit meaningful error messages.

This looks good to me, minor nits inline.

Oct 15 2018, 4:14 AM
philip.pfaffe added a comment to D53270: [NewPM] implement SCC printing for -print-before-all/-print-after-all.

So is this just a bugfix? Or does it add functionality?

Oct 15 2018, 1:11 AM
philip.pfaffe added a comment to D53270: [NewPM] implement SCC printing for -print-before-all/-print-after-all.

What was the deficiency of the initial implementation this is fixing?

Oct 15 2018, 12:29 AM

Oct 11 2018

philip.pfaffe added a comment to D52814: [PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with -fsanitize=address .

You're right, my bad; I missed the fact that it's EP_OptimizerLast, which has no equivalent in the new PM. Your implementation is actually correct here.

Oct 11 2018, 1:20 PM · Restricted Project, Restricted Project
philip.pfaffe accepted D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

This looks great now, thank you!

Oct 11 2018, 7:34 AM · Restricted Project

Oct 10 2018

philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

We're getting there! Some more tiny nits.

Oct 10 2018, 3:54 AM · Restricted Project

Oct 9 2018

philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

I also noticed you got the header in the wrong place. It should live in Transforms/Instrumentation, not in IR.

Oct 9 2018, 4:17 AM · Restricted Project
philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

I see. As far as I can tell, both passes actually use the same options for CompileKernel and Recover when creating new passes, but UseAfterScope is different for both in that the option is passed from a code gen option for AddressSanitizer whereas AddressSanitizerModule sets UseAfterScope as true by default and is dependent on if ASan uses garbage collection friendly instrumentation for globals.

Oct 9 2018, 4:16 AM · Restricted Project

Oct 8 2018

philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

This I'm not sure of since it seems that they both do different things that work independently of each other, though I can't find an example/test where they are used on their own.

I meant just merging the passes. In the sense that you have one pass class with one module run() method and one function run() method. The question then is whether you'll ever want to use different options for the different IRUnits.

Oct 8 2018, 3:06 PM · Restricted Project
philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

Some comments inline. Does it make sense to merge the module and function passes into one? Is it really necessary to distinguish the passes?

Oct 8 2018, 12:34 PM · Restricted Project
philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

See my latest comment. If there is no state to be kept, that state need not be on the exported interface.

Oct 8 2018, 12:12 AM · Restricted Project

Oct 4 2018

philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

Actually I was asking this because as far as I can tell, asan doesn't track state across functions and modules. Which means there's no point in _retaining_ the state, i.e. you don't have to keep an instance of it around.

Oct 4 2018, 2:23 PM · Restricted Project
philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

Well, it will be a bit tricky to avoid moving the interfaces around, since AddressSanitizer contains quite a load of state

Oct 4 2018, 3:07 AM · Restricted Project
philip.pfaffe added a comment to D52814: [PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with -fsanitize=address .

Is this the right place in the pipeline to put the passes? The legacy PM inserts the asan passes at EP_OptimizerLast, why not do the same thing?

Oct 4 2018, 3:05 AM · Restricted Project, Restricted Project

Oct 1 2018

philip.pfaffe added a comment to D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager.

Thank you for moving this along!

Oct 1 2018, 12:53 PM · Restricted Project

Sep 27 2018

philip.pfaffe accepted D51276: [New PM][PassTiming] implement -time-passes for the new pass manager.

LGTM! Some final feedback by @chandlerc would be good though before landing :)

Sep 27 2018, 2:50 AM

Sep 26 2018

philip.pfaffe accepted D52356: [PassTiming] cleaning up legacy PassTimingInfo interface. NFCI..

Okay, all in all this looks alright, and making a single cleanup change seems less verbose than reverting rL340872 first. Please make very clear in this commit message that this is a partial revert and why.

Sep 26 2018, 2:03 AM
philip.pfaffe added inline comments to D51276: [New PM][PassTiming] implement -time-passes for the new pass manager.
Sep 26 2018, 1:57 AM

Sep 25 2018

philip.pfaffe added a reviewer for D51276: [New PM][PassTiming] implement -time-passes for the new pass manager: philip.pfaffe.
Sep 25 2018, 3:08 AM
philip.pfaffe added a comment to D51276: [New PM][PassTiming] implement -time-passes for the new pass manager.

I like how simple this is becoming!

Sep 25 2018, 3:08 AM

Sep 24 2018

philip.pfaffe added a comment to D52356: [PassTiming] cleaning up legacy PassTimingInfo interface. NFCI..

Is that remaining change still useful? Does it make sense to revert instead?

Sep 24 2018, 7:20 AM