This is an archive of the discontinued LLVM Phabricator instance.

[NewPM][CodeGen] Introduce machine pass and machine pass manager
ClosedPublic

Authored by ychen on Sep 17 2019, 7:15 PM.

Details

Summary

Please refer to the RFC here http://lists.llvm.org/pipermail/llvm-dev/2020-July/143309.html

This patch introduces the interfaces of machine passes and machine pass managers.

machine pass could define four methods:
(1) PreservedAnalyses run(MachineFunction &, MachineFunctionAnalysisManager &). Majority of the machine passes use this.
(2) Error doInitialization(Module &, MachineFunctionAnalysisManager &). Passes like AsmPrinter needs a hook to lower/transform global constructs. (invoked before all passes run method)
(3) Error doFinalization(Module &, MachineFunctionAnalysisManager &). Client: PrintMIRPass. This is also for completeness. (invoked after all passes run method)
(4) Error run(Module &, MachineFunctionAnalysisManager &). Client: MachineOutliner, DebugifyMachineModule. I would call this machine module pass which needs a global scope. It is like (2) but subject to pass ordering. Currently, a pass either define this or (1), but not both.

machine pass manger:

  • MachineFunctionAnalysisManager: basically AnalysisManager<MachineFunction> augmented with the ability to register and query IR analyses.
  • MachineFunctionPassManager: support only two methods, addPass and run.

Diff Detail

Event Timeline

ychen created this revision.Sep 17 2019, 7:15 PM
ychen updated this revision to Diff 220708.Sep 18 2019, 11:06 AM

Pass in instead of manage IR unit analysis managers in MachineFunctionIRAnalysisManager

Harbormaster completed remote builds in B38264: Diff 220709.
ychen updated this revision to Diff 233001.Dec 9 2019, 9:56 PM

fix a bug

arsenm accepted this revision.Jan 31 2020, 8:44 AM
arsenm added a subscriber: arsenm.

LGTM with nits

llvm/include/llvm/CodeGen/PassManager.h
58 ↗(On Diff #233001)

Extra space before (

63 ↗(On Diff #233001)

Ditto

llvm/unittests/CodeGen/PassManagerTest.cpp
140

Typo unknonwn

145

Typo unknonwn

This revision is now accepted and ready to land.Jan 31 2020, 8:44 AM

LGTM with nits

@arsenm Thanks for the review! It seems some more folks want to chime in.

ychen updated this revision to Diff 242108.Feb 3 2020, 9:30 AM
  • Address Matt's comments
ychen marked 4 inline comments as done.Feb 3 2020, 9:31 AM

Unit tests: pass. 62418 tests passed, 0 failed and 845 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Any update on this?

ychen added a comment.Jun 22 2020, 7:02 PM

Any update on this?

Thanks for asking. I'm actively sorting out some design issues around this (There are multiple patches to enable preliminary codegen using NPM). I'm planning to send the RFC to the mailing list very soon (by the end of this week hopefully).

ychen updated this revision to Diff 277196.Jul 10 2020, 7:12 PM
  • Update
ychen added a comment.Jul 15 2020, 9:21 PM

I made a few fixes and added some missing functionality in the last update:

  • introduce the machine module pass which runs over an IR module instead of machine function. Clients are on their own to use MMI to do the IR->MIR mapping. This is for passes like MachineOutliner, MachineDebugify. Also added unit test case for this.
  • add PassInstrumentation support
  • add RequireCodeGenSCCOrder (now only a TODO) which is needed for IPRA.
  • simplify SFINAE to use is_detected

@arsenm does this version look good to you?

arsenm added inline comments.Jul 16 2020, 5:49 AM
llvm/lib/CodeGen/MachinePassManager.cpp
36

Braces

54

No else after return

69

Braces

arsenm added inline comments.Jul 16 2020, 5:49 AM
llvm/lib/CodeGen/MachinePassManager.cpp
34

assert(!RequireCodeGenSCCOrder && "not implemented")?

71

Single quotes

80

Braecs

84

Braces

ychen updated this revision to Diff 278540.Jul 16 2020, 10:49 AM
ychen marked 7 inline comments as done.

Address comments

Can you update the commit description to say what's going on?

llvm/include/llvm/CodeGen/MachinePassManager.h
121

Are we still doing the initialization/finalization thing?

ychen retitled this revision from [CodeGen] Define an interface for the new pass manager. (new) to [NewPM][CodeGen] Introduce machine pass and machine pass manager.Jul 16 2020, 2:42 PM
ychen edited the summary of this revision. (Show Details)
ychen marked an inline comment as done.Jul 16 2020, 2:47 PM

Can you update the commit description to say what's going on?

updated.

llvm/include/llvm/CodeGen/MachinePassManager.h
121

I think we have to keep it until we're certain that the alternatives work well, especially for the use cases like AsmPrinter. But that requires we have at least a partially working pipeline with NPM. How about I add a FIXME? We could visit this later.

ychen edited the summary of this revision. (Show Details)Jul 19 2020, 12:47 AM

After thinking about this some more, I still don't understand why MachineFunctions aren't modeled as contained within a Module and don't really see the benefit of all this custom logic. Looking at some legacy passes like MachineOutliner, it's a ModulePass that finds all machine functions itself through the module.
The passes that you gave as examples that have some sort of doInitialization/doFinalization could just become a module pass (I'm thinking about doing that for objc-arc which has a doInitialization)?
Maybe I'm missing something, but going toward the route of modeling modules as containing machine functions and reusing more of the existing infra and less custom stuff specifically for codegen seems better. Then the only thing you'd have to do is add some pass manager/analysis manager typedefs, create a ModuleToMachineFunctionPassAdaptor, and add some proxies to PassBuilder::crossRegisterProxies.
That would make it less confusing and more consistent than having two different run() methods here.
And it would make it easier to share analyses between the different pipelines, by adding the machine function pass manager as part of the overall module pass manager (e.g. in PassBuilder.cpp).

You mentioned

Because their data structures are separated. Logically they are representations of the same/similar thing at different abstraction level.

in the email thread. Do you mean that there should be some sort of MachineModule, instead of Module corresponding to both IR and MIR?

llvm/include/llvm/CodeGen/MachinePassManager.h
20

doInitialization

"like they are"

llvm/lib/CodeGen/MachinePassManager.cpp
48

moulde -> module

llvm/unittests/CodeGen/PassManagerTest.cpp
89

what about tests for returning an Error in doIntialization/doFinalization?

ychen added a comment.Jul 20 2020, 6:47 PM

Thanks for chiming in @aeubanks.

After thinking about this some more, I still don't understand why MachineFunctions aren't modeled as contained within a Module and don't really see the benefit of all this custom logic. Looking at some legacy passes like MachineOutliner, it's a ModulePass that finds all machine functions itself through the module.

MachineOutliner doesn't really need to be a module pass i.e. working on a Module. It just needs a view of all machine functions. Considering it is a rare need and the legacy pass manager does not support this directly. Using ModulePass is a workaround.

The passes that you gave as examples that have some sort of doInitialization/doFinalization could just become a module pass (I'm thinking about doing that for objc-arc which has a doInitialization)?

I think very likely yes. But as I mentioned in the thread, they have some differences in when they are got called. I'm just being defensive here that defining doInitialization/doFinalization in the initial porting phase. If afterward, we have a lot of things working and doInitialization/doFinalization turns out to be not needed, we could remove them easily. But still, I'm open to not keeping it if that's the consensus.

Maybe I'm missing something, but going toward the route of modeling modules as containing machine functions and reusing more of the existing infra and less custom stuff specifically for codegen seems better. Then the only thing you'd have to do is add some pass manager/analysis manager typedefs, create a ModuleToMachineFunctionPassAdaptor, and add some proxies to PassBuilder::crossRegisterProxies.
That would make it less confusing and more consistent than having two different run() methods here.
And it would make it easier to share analyses between the different pipelines, by adding the machine function pass manager as part of the overall module pass manager (e.g. in PassBuilder.cpp).

You mentioned

Because their data structures are separated. Logically they are representations of the same/similar thing at different abstraction level.

in the email thread. Do you mean that there should be some sort of MachineModule, instead of Module corresponding to both IR and MIR?

By saying "their data structures are separated" I meant that modifying/transforming MIR does not cause any change to IR. MIR does not belong to IR class hierarchy. It is not like changing an IR loop also meaning changing its containing IR function or module.

Also, in MIR, there is basically one kind of IR unit (in NPM terminology): machine function. IR has many kinds of IR units: basic block(relatively small scope not so useful for optimization), loop, function, cgscc, module.

In the MIR pipeline (what this patch is about), the IR is not changed and not ran. So all IR analyses results are preserved in MIR pipeline. Modifying MIR could only invalidate MIR analysis results but not IR analyses results.

Based on the above, the requirement for MIR pipeline about IR is that 1) we don't run any IR transformation pass; 2) we ran machine pass initiated IR analyses; 3) we could not invalidate IR analyses.

Defining a proxy class does not do any actual work(the invalidation part is not needed, the rest are just querying the delegated pass manager) although it is definitely conceptually appealing. Likewise, a potential adaptor class (maybe only ModuleToMachineFunctionPassAdaptor as you mentioned) is possible but other than iterating MIR functions from an IR module, it could not do anything else that an adaptor class is supposed to do like aggregating passes preserved analyses and report it back to containing pass manager. The MachineFunctionPassManager is roughly ModuleToMachineFunctionPassAdaptor + PassManager<MachineFunction, MachineFunctionAnalysisManager> combined.

In conclusion, it is possible to use proxy and adaptor classes here and make things work. But IMHO it is unnecessary coupling into IR pass manager infra. Although it is appealing conceptually, it could also cause confusion. Actually, without the doInitialization/doFinalization stuff here, all existing code is necessary even if we changed to use proxy and adaptor classes.

Please let me know if this makes any sense to you. :-)

In the MIR pipeline (what this patch is about), the IR is not changed and not ran. So all IR analyses results are preserved in MIR pipeline. Modifying MIR could only invalidate MIR analysis results but not IR analyses results.

This statement sounds too broad to me. For example, there may not be a 1:1 correspondence from MachineBasicBlock back to an IR BasicBlock, so any control flow analyses would be useless

ychen added a comment.Jul 21 2020, 9:24 AM

In the MIR pipeline (what this patch is about), the IR is not changed and not ran. So all IR analyses results are preserved in MIR pipeline. Modifying MIR could only invalidate MIR analysis results but not IR analyses results.

This statement sounds too broad to me. For example, there may not be a 1:1 correspondence from MachineBasicBlock back to an IR BasicBlock, so any control flow analyses would be useless

Agree. But this does not contradict what I said? Do you mean this (So all IR analyses results are preserved in MIR pipeline.) is broad?

You mentioned

Because their data structures are separated. Logically they are representations of the same/similar thing at different abstraction level.

in the email thread. Do you mean that there should be some sort of MachineModule, instead of Module corresponding to both IR and MIR?

By saying "their data structures are separated" I meant that modifying/transforming MIR does not cause any change to IR. MIR does not belong to IR class hierarchy. It is not like changing an IR loop also meaning changing its containing IR function or module.

Also, in MIR, there is basically one kind of IR unit (in NPM terminology): machine function. IR has many kinds of IR units: basic block(relatively small scope not so useful for optimization), loop, function, cgscc, module.

In the MIR pipeline (what this patch is about), the IR is not changed and not ran. So all IR analyses results are preserved in MIR pipeline. Modifying MIR could only invalidate MIR analysis results but not IR analyses results.

Based on the above, the requirement for MIR pipeline about IR is that 1) we don't run any IR transformation pass; 2) we ran machine pass initiated IR analyses; 3) we could not invalidate IR analyses.

When you say we can't invalidate IR analyses, do you mean we need to assert that any IR analyses that a pass tries to get must not be invalidated? Does that require something like https://reviews.llvm.org/D72893?
Also, something like MachineOutliner::createOutlinedFunction() does create a new Function in order to create a MachineFunction, which seems like it could invalidate IR analyses.

Defining a proxy class does not do any actual work(the invalidation part is not needed, the rest are just querying the delegated pass manager) although it is definitely conceptually appealing. Likewise, a potential adaptor class (maybe only ModuleToMachineFunctionPassAdaptor as you mentioned) is possible but other than iterating MIR functions from an IR module, it could not do anything else that an adaptor class is supposed to do like aggregating passes preserved analyses and report it back to containing pass manager. The MachineFunctionPassManager is roughly ModuleToMachineFunctionPassAdaptor + PassManager<MachineFunction, MachineFunctionAnalysisManager> combined.

I don't understand "it could not do anything else that an adaptor class is supposed to do like aggregating passes preserved analyses and report it back to containing pass manager", can you explain that more?

In conclusion, it is possible to use proxy and adaptor classes here and make things work. But IMHO it is unnecessary coupling into IR pass manager infra. Although it is appealing conceptually, it could also cause confusion. Actually, without the doInitialization/doFinalization stuff here, all existing code is necessary even if we changed to use proxy and adaptor classes.

Please let me know if this makes any sense to you. :-)

ychen added a comment.Jul 21 2020, 3:07 PM

You mentioned

Because their data structures are separated. Logically they are representations of the same/similar thing at different abstraction level.

in the email thread. Do you mean that there should be some sort of MachineModule, instead of Module corresponding to both IR and MIR?

By saying "their data structures are separated" I meant that modifying/transforming MIR does not cause any change to IR. MIR does not belong to IR class hierarchy. It is not like changing an IR loop also meaning changing its containing IR function or module.

Also, in MIR, there is basically one kind of IR unit (in NPM terminology): machine function. IR has many kinds of IR units: basic block(relatively small scope not so useful for optimization), loop, function, cgscc, module.

In the MIR pipeline (what this patch is about), the IR is not changed and not ran. So all IR analyses results are preserved in MIR pipeline. Modifying MIR could only invalidate MIR analysis results but not IR analyses results.

Based on the above, the requirement for MIR pipeline about IR is that 1) we don't run any IR transformation pass; 2) we ran machine pass initiated IR analyses; 3) we could not invalidate IR analyses.

When you say we can't invalidate IR analyses, do you mean we need to assert that any IR analyses that a pass tries to get must not be invalidated? Does that require something like https://reviews.llvm.org/D72893?
Also, something like MachineOutliner::createOutlinedFunction() does create a new Function in order to create a MachineFunction, which seems like it could invalidate IR analyses.

I think it is a special case. IIUC, MachineOutliner is the last machine pass before emitting and it is intended. So even if it is changing IR or invalidating IR analysis, it is not affect anything. If it is paranoid, it could choose to keep IR analyses up-to-date manually if not already.

If passes like MachineOutliner become very common (which I doubt), we should probably keep the existing design and let the pass manually handle IR analyses invaliation.

I gave some thoughts about a potential proxy class between machine function and these IR units. It is kind of hard to implement because you don't know before-hand the implication on IR analyses when MIR analyses is invalidated. The reason is, still, MIR and IR has no much concrete (for example, containing) relationship. If you look at the first few lines of comment above OuterAnalysisManagerProxy/InnerAnalysisManagerProxy, the relationship is required.

Defining a proxy class does not do any actual work(the invalidation part is not needed, the rest are just querying the delegated pass manager) although it is definitely conceptually appealing. Likewise, a potential adaptor class (maybe only ModuleToMachineFunctionPassAdaptor as you mentioned) is possible but other than iterating MIR functions from an IR module, it could not do anything else that an adaptor class is supposed to do like aggregating passes preserved analyses and report it back to containing pass manager. The MachineFunctionPassManager is roughly ModuleToMachineFunctionPassAdaptor + PassManager<MachineFunction, MachineFunctionAnalysisManager> combined.

I don't understand "it could not do anything else that an adaptor class is supposed to do like aggregating passes preserved analyses and report it back to containing pass manager", can you explain that more?

Let say, ModuleToMachineFunctionPassAdaptor which is a module pass. We could use it in two ways: 1) wrap every machine pass in it and run the codegen process as a module pass pipeline (similar goes for a hypothetical FunctionToMachineFunctionPassAdaptor); 2) make the machine pass manager a module pass and insert it in a module pass pipeline.
For 1) I think the adaptor does not do much other than calling the contained machine pass. As a module pass, it does not modify module, it does not invalidate any IR analyses.
For 2) ModuleToMachineFunctionPassAdaptor is almost just a one-time use when setting up the codegen in BackendUtils.cpp/llc/LTO.cpp etc.. It is not generally useful. So we could merge it with the PassManager<MachineFunction, MachineFunctionAnalysisManager> class. I guess you meant this, right?

WDYT?

In conclusion, it is possible to use proxy and adaptor classes here and make things work. But IMHO it is unnecessary coupling into IR pass manager infra. Although it is appealing conceptually, it could also cause confusion. Actually, without the doInitialization/doFinalization stuff here, all existing code is necessary even if we changed to use proxy and adaptor classes.

Please let me know if this makes any sense to you. :-)

ychen added a comment.Jul 21 2020, 3:08 PM

Added @craig.topper if he has any general inputs.

ychen planned changes to this revision.Jul 22 2020, 6:48 PM

Based on the feedbacks from @aeubanks @arsenm @craig.topper , I'll

  • add tests for doing the initialization/finalization. (@aeubanks if you're ok with keeping this, if not, please let me know.)
  • move the testing of target IR passes from driven by llc to opt. This is also in line with what legacy PM does.
  • the pipeline runs all passes on each function / machine function at a time and then the machine function is deleted. This is also what legacy PM does but I missed in the patch.

Suppose we're codegening a TU containing two functions: foo, bar. And the pipeline has two machine passes: pass1, pass2. The order of execution is pass1(on foo), pass2(on foo), <delete>(foo), pass1(on bar), pass2(on bar), <delete>(bar).

If machine module passes like MachineOutiner pass is added into the two-pass pipeline in the middle: pass1, mo_pass, pass2. The order of execution is pass1(on foo), pass1(on bar), mo(on TU), pass2(on foo), <delete>(foo), pass2(on bar), <delete>(bar).

I think it is a special case. IIUC, MachineOutliner is the last machine pass before emitting and it is intended. So even if it is changing IR or invalidating IR analysis, it is not affect anything. If it is paranoid, it could choose to keep IR analyses up-to-date manually if not already.

If passes like MachineOutliner become very common (which I doubt), we should probably keep the existing design and let the pass manually handle IR analyses invaliation.

I gave some thoughts about a potential proxy class between machine function and these IR units. It is kind of hard to implement because you don't know before-hand the implication on IR analyses when MIR analyses is invalidated. The reason is, still, MIR and IR has no much concrete (for example, containing) relationship. If you look at the first few lines of comment above OuterAnalysisManagerProxy/InnerAnalysisManagerProxy, the relationship is required.

Ok I think you're right about MachineOutliner being special. Craig mentioned that MachineFunctions are emitted one at a time for memory reasons, and I've heard that from other people too. MachineOutliner breaks that so it must be special.

I'm still fuzzy on exactly why a "containing" relationship is required, but maybe I'm belaboring this point too much.

Let say, ModuleToMachineFunctionPassAdaptor which is a module pass. We could use it in two ways: 1) wrap every machine pass in it and run the codegen process as a module pass pipeline (similar goes for a hypothetical FunctionToMachineFunctionPassAdaptor); 2) make the machine pass manager a module pass and insert it in a module pass pipeline.
For 1) I think the adaptor does not do much other than calling the contained machine pass. As a module pass, it does not modify module, it does not invalidate any IR analyses.
For 2) ModuleToMachineFunctionPassAdaptor is almost just a one-time use when setting up the codegen in BackendUtils.cpp/llc/LTO.cpp etc.. It is not generally useful. So we could merge it with the PassManager<MachineFunction, MachineFunctionAnalysisManager> class. I guess you meant this, right?

WDYT?

I *think* what you mean by "we could merge it with the PassManager<MachineFunction, MachineFunctionAnalysisManager> class" is what I wanted to say? Putting a pass manager in an adaptor is the normal way of doing things, like at [1].
An adaptor would help with things like https://reviews.llvm.org/D82698.

But maybe since the machine function pass managers returns an Error, it doesn't make as much sense to be able to compose it as a module pass.

[1]: https://github.com/llvm/llvm-project/blob/724bf4ee23a3993689d55afb2845949e05c83b1c/llvm/lib/Passes/PassBuilder.cpp#L792

I think I'm slightly more on board with this than before, although I would like somebody like asbirlea to also take a look.

IIRC the MIR pipeline is essentially flat (vs the IR one), so at this point it's probably fine to not have the adaptor complexity. However, if this changes to become more than a one time use, then perhaps the Module-Function hierarchy makes sense.
Let me ask this differently, if the CodegenNPM introduced the ModuleToFunctionPassAdaptor now, and it was only used during codegen set-up (assuming negligible overhead for this complexity), is it possible it will be useful long-term in case there are changes in the current restrictions?

It would be very helpful if you added an overall comment explanation (or adding to the documentation) on the decision of which parts of the IR NPM infra is used in the CodegenNPM.

llvm/include/llvm/CodeGen/MachinePassManager.h
46

s/A/An

llvm/lib/CodeGen/MachinePassManager.cpp
36

I'm assuming this and other places meant remove braces for single instruction for/if?

arsenm added inline comments.Jul 23 2020, 12:53 PM
llvm/lib/CodeGen/MachinePassManager.cpp
36

I meant add for multiline

ychen added a comment.Jul 23 2020, 3:46 PM

IIRC the MIR pipeline is essentially flat (vs the IR one), so at this point it's probably fine to not have the adaptor complexity. However, if this changes to become more than a one time use, then perhaps the Module-Function hierarchy makes sense.
Let me ask this differently, if the CodegenNPM introduced the ModuleToFunctionPassAdaptor now, and it was only used during codegen set-up (assuming negligible overhead for this complexity), is it possible it will be useful long-term in case there are changes in the current restrictions?

TBH, I don't think the adaptor class would be useful for cases other than codegen setup, because a partial MIR pipeline seems not very useful to be run as part of an IR pipeline. The only other place, I could think of, the adaptor could be useful is to concatenate the pre-codengen IR pipeline with the MIR pipeline. But similar to the possibility of combining opt-pipeline with codegen pipeline, I'm not sure it brings actual benefits other than aesthetics.

It would be very helpful if you added an overall comment explanation (or adding to the documentation) on the decision of which parts of the IR NPM infra is used in the CodegenNPM.

Sure, will do.

IIRC the MIR pipeline is essentially flat (vs the IR one), so at this point it's probably fine to not have the adaptor complexity. However, if this changes to become more than a one time use, then perhaps the Module-Function hierarchy makes sense.
Let me ask this differently, if the CodegenNPM introduced the ModuleToFunctionPassAdaptor now, and it was only used during codegen set-up (assuming negligible overhead for this complexity), is it possible it will be useful long-term in case there are changes in the current restrictions?

TBH, I don't think the adaptor class would be useful for cases other than codegen setup, because a partial MIR pipeline seems not very useful to be run as part of an IR pipeline. The only other place, I could think of, the adaptor could be useful is to concatenate the pre-codengen IR pipeline with the MIR pipeline. But similar to the possibility of combining opt-pipeline with codegen pipeline, I'm not sure it brings actual benefits other than aesthetics.

Thank you for clarifying. Let's move forward with the current design and document it to keep a record in case it needs to be revisited in the future. It doesn't sound like it will be the case at this point, but it's nice to have the doc to point to :-).

It would be very helpful if you added an overall comment explanation (or adding to the documentation) on the decision of which parts of the IR NPM infra is used in the CodegenNPM.

Sure, will do.

llvm/lib/CodeGen/MachinePassManager.cpp
36

Ah I see, I didn't see this in the styleguide. I always favored not having the braces for single statement if it doesn't affect readability, but either works here I guess.

ychen updated this revision to Diff 282745.EditedAug 3 2020, 3:33 PM
ychen edited the summary of this revision. (Show Details)
  • Add unit tests for doInit/doFinal and machine module pass, also checks for pass order.
  • Address other comments.
  • D85168 add support testing codegen IR passes with opt
  • D83612 has test cases showing that FreeMachineFunction is called properly.
This revision is now accepted and ready to land.Aug 3 2020, 3:33 PM
ychen requested review of this revision.Aug 3 2020, 3:51 PM
asbirlea added inline comments.Aug 3 2020, 5:01 PM
llvm/include/llvm/CodeGen/MachinePassManager.h
20

s/doInitilization/doInitialization

73

doxygen comment here and below?

llvm/lib/CodeGen/MachinePassManager.cpp
45

I think it's clearer if this is a do{}while(true) with the Idx and Size initialization outside the loop.

65

Use Idx directly below?

76
for (;Begin != Idx; ++Begin) {
  ... Passes[Begin]
...
aeubanks accepted this revision.Aug 4 2020, 4:15 PM

LGTM

This revision is now accepted and ready to land.Aug 4 2020, 4:15 PM
ychen updated this revision to Diff 283324.Aug 5 2020, 12:07 PM
  • Beef up comments, use doxygen format and move them to proper places
  • Address other comments
asbirlea accepted this revision.Aug 6 2020, 1:09 PM
This revision was landed with ongoing or failed builds.Aug 7 2020, 11:01 AM
This revision was automatically updated to reflect the committed changes.
ychen reopened this revision.Dec 9 2020, 12:11 PM
This revision is now accepted and ready to land.Dec 9 2020, 12:11 PM
ychen updated this revision to Diff 310605.EditedDec 9 2020, 12:11 PM

fix the laying issue.

  • change buildCodeGenPipeline signature to take pass manager references as inputs.
ychen requested review of this revision.Dec 14 2020, 9:24 AM

@dblaikie The laying issue in 31ecf8d29d81d196374a562c6d2bd2c25a62861e is addressed here. Do you mind taking another look? Thanks!

@dblaikie The laying issue in 31ecf8d29d81d196374a562c6d2bd2c25a62861e is addressed here. Do you mind taking another look? Thanks!

The last delta on the patch is fairly large - not sure I can absorb it all. If the problematic dependency is no longer present, that's the key thing - I'm happy to take your word for it, if you've checked the included headers/grep'd for references of whichever libraries were incorrectly connected.

ychen added a comment.Dec 14 2020, 6:20 PM

@dblaikie The laying issue in 31ecf8d29d81d196374a562c6d2bd2c25a62861e is addressed here. Do you mind taking another look? Thanks!

The last delta on the patch is fairly large - not sure I can absorb it all. If the problematic dependency is no longer present, that's the key thing - I'm happy to take your word for it, if you've checked the included headers/grep'd for references of whichever libraries were incorrectly connected.

Yeah, I've checked these. Pretty sure there are no new libraries and their headers are used.

aeubanks requested changes to this revision.Dec 21 2020, 9:35 AM

The latest patch looks like it adds a lot more than the original change, is that intended?

This revision now requires changes to proceed.Dec 21 2020, 9:35 AM
ychen added a comment.Dec 21 2020, 9:52 AM

The latest patch looks like it adds a lot more than the original change, is that intended?

No, that's not intended. You're absolutely right. I somehow reopen the wrong patch. This patch is fine (need no amendment). I should've reopen D83608.

ychen added a comment.Dec 21 2020, 9:55 AM

I'll update this to the correct/landed version. Sorry for the noise.

ychen updated this revision to Diff 313139.Dec 21 2020, 10:01 AM
  • update to the landed version.
ychen accepted this revision.Dec 21 2020, 10:04 AM
aeubanks accepted this revision.Dec 21 2020, 10:26 AM
This revision is now accepted and ready to land.Dec 21 2020, 10:26 AM

LLVMBuild.txt has been deleted. The dependencies are entirely specified by CMakeLists.txt

LLVMBuild.txt has been deleted. The dependencies are entirely specified by CMakeLists.txt

This patch was committed when LLVMBuild.txt is still being used. Here is just to put the reviewed patch into its landed's version after I did a wrong patch update above.

ychen closed this revision.Dec 21 2020, 10:36 AM