atrick (Andrew Trick)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 2 2012, 11:45 PM (292 w, 6 d)

Recent Activity

May 23 2018

atrick accepted D46838: [MachineScheduler] Clear subregister entries also in addPhysRegDeps when handling a def.

LGTM, ignore my previous comment and sorry for the delay.

May 23 2018, 1:04 PM

May 18 2018

atrick closed D46841: MCSchedModel: Add comments to IssueWidth..

Landed here

May 18 2018, 9:07 AM
atrick committed rL332729: MCSchedModel: Add comments to IssueWidth..
MCSchedModel: Add comments to IssueWidth.
May 18 2018, 9:02 AM

May 15 2018

atrick added a comment to D46801: add getAA in ScheduleDAG for using in HazardRec.

@hfinkel This is part of an alternate solution for https://reviews.llvm.org/D46243.

May 15 2018, 4:09 PM
atrick updated the diff for D46841: MCSchedModel: Add comments to IssueWidth..
May 15 2018, 4:01 PM
atrick resigned from D46801: add getAA in ScheduleDAG for using in HazardRec.

Just to be clear, you aren't actually using PostRASchedulerList are you?

May 15 2018, 10:09 AM

May 14 2018

atrick created D46841: MCSchedModel: Add comments to IssueWidth..
May 14 2018, 12:21 PM
atrick added a comment to D46838: [MachineScheduler] Clear subregister entries also in addPhysRegDeps when handling a def.

We didn't support cross-call scheduling last I looked at this. Hopefully @MatzeB can verify that your fix is correct.

May 14 2018, 12:09 PM
atrick updated subscribers of D46837: [MachineScheduler] Skip an implicit def of a super-reg added by regalloc in findDefIdx..

That's interesting. @MatzeB is this the right fix? Is regalloc supposed to be adding implicit defs in front of the uses, and don't we/shouldn't we have a general API to handle this?

May 14 2018, 10:32 AM
atrick accepted D46817: change passing ScheduleDAG with ScheduleDAGInstrs when creating HazaradRec.

Fine with me. I don't know if it's possible anyone still uses ScoreBoardHazardRecognizer with SelectionDAG scheduler, but they really need to stop doing that.

May 14 2018, 10:28 AM

May 13 2018

atrick added a comment to D46243: Move Schedule class to header file for allowing inheritance.

ScheduleDAGMI is defined in MachineScheduler.h. Maybe there's some other reason the API should take ScheduleDAG, but I'm not aware of it. I think that base class is only to share implementation across SelectionDAG and MI scheduling.

May 13 2018, 7:42 PM
atrick added a comment to D46801: add getAA in ScheduleDAG for using in HazardRec.

If getAA may return NULL, that should be mentioned in the comment.

May 13 2018, 7:36 PM
atrick accepted D46801: add getAA in ScheduleDAG for using in HazardRec.

LGTM

May 13 2018, 7:35 PM

May 11 2018

atrick added a comment to D46695: [RFC] [Patch 1/3] Add a new class of predicates for variant scheduling classes..

A few minor questions about naming conventions.

May 11 2018, 10:25 AM

May 7 2018

atrick added a comment to D46243: Move Schedule class to header file for allowing inheritance.

Hi Andy,

thanks for your suggestion and remarks.
I think passing DAGMI(ScheduleDAGInstrs) instead of DAG can be the first step for my solution, then I will also need to add some functions to the interface of AliasAnalisys (some generic functions like getDistanceBetweenMemorys(MIa,MIb)...). I hope this will be accepted.

but the problem is that AliasAnalysis *AAForDep; is 'protected' inside the DAGI, do you think its acceptable to add getAA() to the interface of the DAGMI ?

thanks,
Atheel

May 7 2018, 10:20 AM

May 3 2018

atrick added a comment to D46243: Move Schedule class to header file for allowing inheritance.

thanks for your comment,
yes, I need to reuse "scheduleRegions" code. why its so important to avoid implicit coupling between the target code and machine independent code in this class?

I’m implementing an optimization pass analysis that will be used from the TargetHazardRecognizer, so I must pass its result in the constructor of the HazaradRecognizer.
I have encountered some open source classes that I need to expand (using inheritance) but couldn’t due to some reasons:

  1. Expanding “MachineSchedulerBase” from MachineScheduler.cpp: I want to inherit it and expand it (for changing the call of CreateTargetPostRAHazardRecognizer)but I can’t because its declared in cpp file not in header
  2. the same as above for PostRASchedule pass.
  3. Expanding BasicAAResult from BasicAliasAnalysis.h: Its inheriting the “AAResultBase<BasicAAResult>” using “Mixin”, it looks like its not designed for been inherited again.

    • In BasicAAResult I have added “CheckBankConflict” functions that uses functions from this class (need to re-design it into TargetBankConflict.cpp) • In MachineSchedulerBase I want to change the call for the HazardRecognizer. I need to pass the BasicAAResults when creating the HazaradRecognizer: llvm::createTargetHazardRecognizer(MachineFunction &MF) (I cant created it inside the HazaradRecognizer because its not a pass) so I have changed it into: llvm::createTargetHazardRecognizer(MachineFunction &MF, BasicAAResult *BCA) and I need to change the functions declaration and implementation that calls it. (functions from (class MachineScheduler : public MachineSchedulerBase ) and more… so I want to:
  4. Create TaregtBankConflict.h pass and pass its result to the CXDHazaradRecognizer. For that I will need to: • Inherit BasicAAResult for avoiding code copy (I have no solution for it). • Inherit MachineScheduler for avoiding code copy. (my suggestion is to move it to the header file)
May 3 2018, 3:09 PM

Apr 30 2018

atrick added a comment to D46243: Move Schedule class to header file for allowing inheritance.

These classes are not meant to be inherited from, and inheritance is generally terrible way to achieve code reuse across components. The principle behind the scheduling interfaces is to avoid implicit coupling between the target code and machine independent code.

Apr 30 2018, 12:53 AM

Feb 26 2018

atrick accepted D43329: [SystemZ, MachineScheduler] Refactor GenericScheduler::tryCandidate() to reuse parts in a new SystemZ scheduling strategy..

This looks fine to me based on a quick review. I don't know if @fhahn or @MatzeB still want to weigh in. Not sure if anyone else needs to review the SystemZ specific code or if you effectively own that.

Feb 26 2018, 10:54 AM

Feb 21 2018

atrick added a comment to D43329: [SystemZ, MachineScheduler] Refactor GenericScheduler::tryCandidate() to reuse parts in a new SystemZ scheduling strategy..

Just to express my thoughts: I see this point in the sense that if a target truly had a perfect set-in-stone tuning, it would be disastrous to change anything in an uncontrollable way. But given that mischeduler is relatively new and evolving, and a target may merely have been able to improve benchmarks with a minor modification, I think it's more natural to think that a target would really want to be in on the improvements to come in the future. In other words, *not* to decouple. Take register pressure, for instance. I don't think a target that does not have it's own register pressure heuristics would want to fall behind if the common code changes in the future. That change should then be general goodness, or it should be put in a target specific strategy, right?

So to me personally, working on just one backend, it would be slightly preferred to have e.g. the tryCandidate_RegPress() function in the target strategy, so that if somebody improved it, my target would immediately get that improvement. I don't want to decouple this, since I was merely adding a heuristic with lesser priority.

On the other hand, providing just the smaller utility functions and then doing a copy-and-paste of tryCandidate() should probably work quite well in practice, as you say. In the very long run, this would also give maximum flexibility for each target.
I suppose then we could just have one or two of those new methods, like tryCandidate_Clustered_Weak().

Feb 21 2018, 10:46 AM

Feb 20 2018

atrick added a comment to D38351: MIScheduler improved handling of copied physregs.

Thanks for CC'ing me, but I can't comment on whether this is the right heuristic since I haven't done this level of tuning in a long time. Deferring to @MatzeB.

Feb 20 2018, 6:34 PM
atrick added a comment to D43329: [SystemZ, MachineScheduler] Refactor GenericScheduler::tryCandidate() to reuse parts in a new SystemZ scheduling strategy..

General thoughts:

Feb 20 2018, 6:31 PM

Feb 14 2018

atrick added a comment to D43235: [SchedModel] Complete models shouldn't match against itineraries when they don't use them (PR35639) (WIP).

I can confirm that the purpose of ItinRW is to avoid the need to define InstRW for a subtarget when the target already had itinerary classes. (Things would be *a lot* simpler if all the subtargets used the same style of machine description.)

Feb 14 2018, 12:50 PM

Nov 22 2017

atrick added a comment to D39805: [Power9] Set MicroOpBufferSize for Power 9.

Obviously I meant "With MicroOpsBufferSize >1 (OOO)" above.

Nov 22 2017, 5:35 PM
atrick added a comment to D39805: [Power9] Set MicroOpBufferSize for Power 9.

Think of CurrCycle as the cycle in which an instruction is issued.

Nov 22 2017, 5:34 PM

Sep 7 2017

atrick added a comment to D33960: ErrorHandling: report_fatal_error: call abort() instead of exit().

Note that you can get the stack trace just call RunSignalHandlers before calling exit(1)...

Sep 7 2017, 5:14 PM

Sep 6 2017

atrick added a comment to D33960: ErrorHandling: report_fatal_error: call abort() instead of exit().

Please add a comment that abort() also causes any signal handlers registered with LLVM such as PrettyStackTrace to run, which is important for compiler debugging.

Sep 6 2017, 4:46 PM

Aug 16 2017

atrick added a comment to D35053: Improve post-RA scheduling for SystemZ.

Andy: Do the comments now look ok?

Aug 16 2017, 12:57 PM

Aug 7 2017

atrick added a comment to D36381: [MISched] Add enableMachineScheduler function that checks enable-misched..

It sounds to me like you want a "MI-Sched Mode" convenience flag that does more than what it claims to do at face value, rather than needing to specify all the individual flags for SelectionDAG heuristic, enabling MI-Sched, coalescing heuristic. I'm not going to nit pick on the flag names because I don't have any stake it in. But I think you need to discuss this on llvm-dev and argue it out taking into consideration all of the goals:

Aug 7 2017, 12:39 PM
atrick added a comment to D36381: [MISched] Add enableMachineScheduler function that checks enable-misched..

I don't understand the motivation here. The purpose of command line flags is to test functionality in isolation. If you want a test case to control both SelectionDag and MISched functionality, then you use two command line flags.

Aug 7 2017, 10:47 AM

Jul 28 2017

atrick added inline comments to D35053: Improve post-RA scheduling for SystemZ.
Jul 28 2017, 10:54 AM

Jul 20 2017

atrick accepted D33818: [ScheduleDAG] Don't schedule node with physical register interference.

LGTM. Thanks.

Jul 20 2017, 2:02 PM

Jun 27 2017

atrick accepted D34675: Fix incorrect comment in machine-scheduler.

LGTM

Jun 27 2017, 9:16 AM

Jun 12 2017

atrick added a comment to D30446: [IndVars] Do not branch on poison.

Thanks for doing the performance validation @mzolotukhin.
Nice code @sanjoy.

Jun 12 2017, 11:40 PM

Jun 5 2017

atrick added a comment to D30446: [IndVars] Do not branch on poison.

As a generally good direction I like the idea of moving LFTR, and eventually even IV widening, forward to the LSR stage of the pipeline.
So, I'm ok with it as long as you do the normal amount of benchmark validation and @mzolotukhin doesn't see any immediate performance issues with this approach.

Jun 5 2017, 1:19 PM

May 9 2017

atrick accepted D32802: (Was: Add checks so that -pre-RA-sched=list-ilp does not crash on SystemZ (Bug 32723). ->) Implement getRepRegClassFor() in SystemZ..

I'm haven't looked at this code in a long time, and I have no idea if it makes sense to have an Untyped MVT at this point in the code. But... your fix looks harmless.

May 9 2017, 12:00 AM

May 5 2017

atrick added a comment to D32563: Add LiveRangeShrink pass to shrink live range within BB..

Can you collect spill code stats and benchmark scores from the test suite?
Can you get someone to run benchmarks on arm64 as well?

May 5 2017, 9:24 PM

May 2 2017

atrick added a comment to D32563: Add LiveRangeShrink pass to shrink live range within BB..

The time to shrink live ranges is after all the global machine code
motion is done if the goal is handle as many cases as possible.

May 2 2017, 6:26 PM
atrick added a comment to D32563: Add LiveRangeShrink pass to shrink live range within BB..

I think the algorithm needs to be explained. It seems reasonable to argue that single def/use live ranges should be kept short. But is there any reason at all to run this before other IR passes? It seems natural to run this after machine LICM and Sinking.

May 2 2017, 5:49 PM
atrick added a comment to D32563: Add LiveRangeShrink pass to shrink live range within BB..

Ideally there should be a separate pass that runs on the SSA machine code (before register coalescing) to minimize register pressure and hide latency for chains of loads or FP ops. It should work across calls and loop boundaries. You could even do a kind of poor-man's cyclic scheduling this way.

May 2 2017, 3:50 PM
atrick added a comment to D32563: Add LiveRangeShrink pass to shrink live range within BB..

Why are the adds "sunk down" in the first place? Is this reassociation at work?

May 2 2017, 1:46 PM

May 1 2017

atrick updated subscribers of D30446: [IndVars] Do not branch on poison.

My suggestion was this: LFTR is not necessary this early in the pipeline if it's only purpose is converting a 'cmp slt' to 'cmp eq'. As long as SCEV can compute the trip count, the loop test shouldn't matter. If we really need to convert the loop test, it could be done during LSR and we could drop the 'nw' flags at that point.

May 1 2017, 9:34 AM

Apr 24 2017

atrick accepted D32105: [IVUsers] don't bail out of normalizing non-affine add recs.

Yeah, it seems like you should have the positive version of this test.
Otherwise, lgtm.

Apr 24 2017, 10:56 PM

Apr 22 2017

atrick added inline comments to rL300311: This patch closes PR#32216: Better testing of schedule model instruction….
Apr 22 2017, 12:59 AM

Apr 21 2017

atrick added inline comments to rL300311: This patch closes PR#32216: Better testing of schedule model instruction….
Apr 21 2017, 2:40 PM
atrick accepted D32104: Teach SCEV normalization to de/normalize non-affine add recs.

Neat. I wouldn't usually add complexity for an unexpected situation, but this is purely a generalization.

Apr 21 2017, 10:49 AM

Apr 10 2017

atrick added a comment to D30951: [AArch64] Simplify MacroFusion.

This is probably fine, but I can't look at it closely at the moment. @MatzeB did some work in this area recently.

Apr 10 2017, 11:13 AM

Mar 31 2017

atrick added a comment to D31536: [SelectionDAG] Check CALLSEQ_BEGIN nodes in DelayForLiveRegs.

I don't remember (or never understood) how all this code works. But the fix looks reasonable.

Mar 31 2017, 10:11 AM

Mar 26 2017

atrick accepted D30744: Improve machine schedulers for in-order processors.

LGTM. I think you should comment in TargetSchedule.td that SingleIssue is an alias for Begin/EndGroup.

Mar 26 2017, 1:32 PM

Mar 25 2017

atrick added a reviewer for D30350: [LSR] Add a cap for reassociation of AllFixupsOutsideLoop type LSRUse to protect compile time: sanjoy.
Mar 25 2017, 6:29 PM
atrick added a comment to D30744: Improve machine schedulers for in-order processors.

That looks good. But this version of the patch no longer has the SingleIssue flag support in the SubtargetEmitter. I thought you wanted that so the feature was more self-documenting. Your previous implementation looked ok to me. I just don't think you need any flag in the implementation of the model (MCSchedule.h).

Mar 25 2017, 6:24 PM
atrick added a comment to D30350: [LSR] Add a cap for reassociation of AllFixupsOutsideLoop type LSRUse to protect compile time.

@qcolombet, thanks for the analysis. It would be even better if we had a fix to the SCEVExpander's pathological behavior. Now I'm afraid this patch just hides the problem for this particular category of test case.

Mar 25 2017, 11:36 AM

Mar 22 2017

atrick requested changes to D30744: Improve machine schedulers for in-order processors.

I don't think you need the SingleIssue flag in MCSchedule.h any more.

Mar 22 2017, 10:52 AM

Mar 17 2017

atrick added a reviewer for D31081: [ARM] ScheduleDAGRRList::DelayForLiveRegsBottomUp must consider OptionalDefs: MatzeB.
Mar 17 2017, 1:05 PM
atrick added a comment to D30744: Improve machine schedulers for in-order processors.

I just looked at the MachineScheduler implementation. I see this TODO:
/// TODO: Also check whether the SU must start a new group.

Mar 17 2017, 9:10 AM
atrick added a comment to D30744: Improve machine schedulers for in-order processors.

I think a SingleIssue flag would be a good user option. Representing such a flag in the tables generated by the machine model (MCSchedClassDesc) would be redundant. Either begin/endgroup already does the same thing, or it's a bug that should be fixed.

Mar 17 2017, 8:31 AM

Mar 16 2017

atrick added a comment to D30446: [IndVars] Do not branch on poison.

I was referring to the same example but missed that there were two stores. So FindLoopCounter is working as intended.

Mar 16 2017, 11:24 AM
atrick added a comment to D30446: [IndVars] Do not branch on poison.

Ok. I thought the problem was caused by converting the loop test to a post-increment compare. Is that a problem?

Mar 16 2017, 10:49 AM
atrick added a comment to D30446: [IndVars] Do not branch on poison.

When I talk about creating or removing an IV, I'm talking about the strongly connected phi cycle. I'm not referring to any "derived IV" stemming from values that depend on the IV.

Mar 16 2017, 9:07 AM

Mar 15 2017

atrick added a comment to D30446: [IndVars] Do not branch on poison.

Materializing a new IV isn't a good canonicalization. It may block other loop transformations and there's no guarantee that LSR will undo it. General rule: never pessimize your loop and expect LSR to rewrite it. LSR only works well on a small subset of loops and essentially gives up in a lot of cases. I agree the complexity smells bad. I don't have a better suggestion at the moment.

Mar 15 2017, 5:46 PM
atrick added a comment to D30446: [IndVars] Do not branch on poison.

I see this transform as a "canonicalization" type transform (supposed to make later loop opts easier), but I did not add it. Maybe @atrick can shed some light here on the initial motivation? I'm most certainly in favor of deleting code where possible.

Mar 15 2017, 3:43 PM

Mar 14 2017

atrick accepted D30626: MachineScheduler/ScheduleDAG: Add support for GetSubGraph.

This seems reasonable to me. Hopefully the linker will strip utilities like this for build targets that don't need them.

Mar 14 2017, 1:59 PM

Mar 9 2017

atrick added a comment to D30744: Improve machine schedulers for in-order processors.

Separate processor resources should be defined to model issue resources rather than trying to work around it by consuming functional units. The processor resources were always intended to be used for both issue resources and functional units. It just isn't well documented or easily discoverable. It might makes sense to have either a single issue flag or a separate list of issue resources in the tablegen machine model. My argument is that the scheduler itself already supports this functionality.

Mar 9 2017, 10:49 AM

Mar 8 2017

atrick added a comment to D30744: Improve machine schedulers for in-order processors.

That said, if there are multiple target maintainers who think adding a special SingleIssue case would be much better than the alternatives, then it may be worth adding.

Mar 8 2017, 4:13 PM
atrick added a comment to D30744: Improve machine schedulers for in-order processors.

I think what Matthias was getting at is that you can add a new ProcResource to model issue constraints. If you have a dual issue machine then any instruction that can't be grouped would simply consume both issue resources. Another option might be to use the begin/end group flags.

Mar 8 2017, 4:10 PM
atrick added a comment to D30350: [LSR] Add a cap for reassociation of AllFixupsOutsideLoop type LSRUse to protect compile time.

LSR is fundamentally combinatorial. It relies on arbitrary pruning. I don't like it, but that's the way it is, and we need to guard against pathological cases. I just wonder why you complete bypass the reassociation code instead of simply limiting the number of expressions that participate.

Mar 8 2017, 1:48 PM

Feb 28 2017

atrick added a comment to D30477: [SCEV] Compute affine range in another way to avoid bitwidth extending..

I haven't fully reviewed the code, but I'm really glad you were able to express the range without extending its width. Thanks for doing this.

Feb 28 2017, 5:02 PM

Feb 7 2017

atrick added a comment to D26429: [LSR] Allow formula containing Reg for SCEVAddRecExpr with loop other than current loop.

Thanks.

Feb 7 2017, 9:38 PM

Jan 20 2017

atrick added a comment to D28489: [CodeGen] Move MacroFusion to the target.

Note that adjustSchedDependency is defined as updating the latency. It's very important for target hooks not to mutate data structures people's backs.

Jan 20 2017, 11:20 AM

Jan 18 2017

atrick added a comment to D28489: [CodeGen] Move MacroFusion to the target.

I think you should find a way to do this without calling shouldScheduleAdjacent on every DAG node. It's fine to say you've tested compilation time but what really matters here are the pathological cases with very large blocks. It's rare for instructions in the middle of blocks to have fusion opportunities, so it's wrong to introduce this potential cost for all blocks.

Jan 18 2017, 11:40 AM

Jan 15 2017

atrick accepted D25318: [DAG] Don't increase SDNodeOrder for dbg.value/declare..

In principle this makes sense to me. Go ahead if there are no objections from @bogner or @aprantl.

Jan 15 2017, 10:24 AM

Jan 13 2017

atrick accepted D28629: [SCEV] Limit recursion depth of constant evolving.

Sounds good to me. I don't think you need to add a command line flag.

Jan 13 2017, 10:04 AM

Jan 12 2017

atrick added a comment to D28629: [SCEV] Limit recursion depth of constant evolving.

Rather than a command line option, can you find the recursion limit hit by the LLVM test suite (and SPEC if you have access) and set the limit a bit higher with sufficient comments?

Jan 12 2017, 4:13 PM

Jan 9 2017

atrick added a comment to D28489: [CodeGen] Move MacroFusion to the target.

Yep, this could all be done in the target. SDep::Cluster is effectively a scheduler API for the subtarget to use as it wishes.

Jan 9 2017, 4:40 PM
atrick added a comment to D28489: [CodeGen] Move MacroFusion to the target.

Would it make sense to have a TII.mayFuseWithPrecedingInstr() to avoid testing all DAG edges? The DAG is quadratic, but this a rare opportunity.

Jan 9 2017, 3:30 PM
atrick added a comment to D28488: [CodeGen] Implement the SUnit::print() method.

Looks reasonable to me.

Jan 9 2017, 3:22 PM

Jan 3 2017

atrick accepted D28171: Enable disabled loopidiom test. Apparently we handle it now.

Thanks Xin!

Jan 3 2017, 10:41 AM

Dec 12 2016

atrick added a comment to D27216: [SCEVExpand] do not hoist divisions by zero (PR30935).

How do you know this won't move division that used to be outside the loop (possibly multiple levels) inside the innermost loop?

Dec 12 2016, 8:23 AM

Dec 9 2016

atrick resigned from D27592: Reimplement depedency tracking in the ImplicitNullChecks pass.

Sounds great, but I 'm unavailable for code reviews for about the next month.

Dec 9 2016, 3:59 AM

Nov 22 2016

atrick accepted D26986: MachineScheduler: Export function to construct "default" scheduler..

This change is consistent with the original intention of the design.

Nov 22 2016, 4:30 PM

Nov 8 2016

atrick added a comment to D26185: [ARM] Loop Strength Reduction crashes when targeting ARM or Thumb..

Thanks @efriedma.

Nov 8 2016, 11:17 AM

Nov 4 2016

atrick added a reviewer for D26299: Improving the efficiency of the Loop Unswitching pass: mzolotukhin.
Nov 4 2016, 2:45 PM

Oct 31 2016

atrick accepted D26156: Fix per-processor model scheduler definition completeness check.

Thanks. This is great, as long as you tested all in-tree targets.

Oct 31 2016, 10:16 AM

Oct 18 2016

atrick added a comment to D17260: SystemZ scheduling implementation.

Does anyone know how to say "Instruction B should have the same scheduling class as instruction A" ? This was the only point I could not get fixed.

I'm not sure I understand the problem. I *think* you can mark your scheduling model "complete", then add InstAlias records in your .td file without adding new InstRW records...

This is not about InstAlias records (which are aliases for the assembler) -- those indeed work fine. The issue here was about aliases for the code generator. We use those usually because some instructions need operands on the MC level that we don't want to expose on the MI level. For example, a "return" instruction on SystemZ is actually a "br %r14", but at the MI level this doesn't have any operands, but is simply defined as an alias:

def Return : Alias<2, (outs), (ins), [(z_retflag)]>;

where Alias is formally an Instruction, but doesn't have any information about mnemonic or opcodes. Instead, when this alias is about to be emitted, we emit an actual BR instruction pattern, adding the R14 operand at this point.

This means that to get a complete scheduler model, we have to duplicate the scheduling info for BR also for Return. It would be nice if there were instead a way to say in the definition of Return to just look at BR for scheduling info.

Oct 18 2016, 10:57 AM
atrick added a comment to D17260: SystemZ scheduling implementation.
Oct 18 2016, 10:06 AM

Oct 17 2016

atrick committed rL284452: Improve tablegen gen-subtarget diagnostics for missing machine models..
Improve tablegen gen-subtarget diagnostics for missing machine models.
Oct 17 2016, 9:27 PM
atrick added a comment to D24855: MachineScheduler: Enable macro fusion in post-RA scheduler.

On x86 it's meant to be a heuristic. If you think all subtargets should instead force macro-fusion before scheduling (and have benchmarks to prove it) then we should delete the code that implements the heuristic.

Oct 17 2016, 8:44 PM
atrick added a comment to D24855: MachineScheduler: Enable macro fusion in post-RA scheduler.

Doesn't this force macro fusion for all targets/subtargets? If we wanted to do that, we wouldn't need the cluster edge and scheduler heuristic anymore. Shouldn't there be a TII->forceMacroFusion() option?

Oct 17 2016, 8:33 PM
atrick accepted D25140: ScheduleDAGInstrs: Add condjump deps in addSchedBarrierDeps().

LGTM.
Thanks!

Oct 17 2016, 6:33 PM

Sep 28 2016

atrick added a comment to D24855: MachineScheduler: Enable macro fusion in post-RA scheduler.

Marking a DAG node as "dead" for the purpose of scheduling should be an easy thing to do, relative to supporting instruction bundles. But adding the extra DAG edges is also a fine solution, just not quite as direct.

Sep 28 2016, 10:01 PM

Sep 12 2016

atrick added a comment to D21398: [IVUser] Add a trunc if used only by a single IV user.

It would be great to fix the example shown in the summary. However, I'm concerned that the proposed heuristic could hurt codegen in other cases.The cases where this heuristic generates worse code should be identified to determine whether this is the right strategy.

Sep 12 2016, 1:24 PM
atrick added a comment to D21398: [IVUser] Add a trunc if used only by a single IV user.

Does this patch still need review, or has it been subsumed by other changes?

Sep 12 2016, 11:39 AM

Aug 18 2016

atrick added a comment to D23679: MachineScheduler: Make some GenericScheduler member variables protected.

Whatever works for you. I was thinking custom schedulers would use composition of utilities rather than inheriting the whole set of heuristics. It comes down to whether you want someone tuning the GenericScheduler to impact performance on your target. They will have no way to evaluate it.

Aug 18 2016, 6:21 PM

Jun 23 2016

atrick accepted D21448: Codegen: LICM Remove check for exactly 1 register def..

LGTM

Jun 23 2016, 2:18 PM
atrick accepted D21627: Codegen: [X86] preservere memory refs for folded umul_lohi.

LGTM

Jun 23 2016, 2:18 PM

Jun 21 2016

atrick added a comment to D21448: Codegen: LICM Remove check for exactly 1 register def..

Frankly I don't understand what MID.getNumDefs() has to do with anything here. So your change looks ok to me. But I'm not clear on how the register form of the instruction ends up with multiple defs--I guess it already had multiple defs. Can you show an example of that in the form of machine instrs?

Jun 21 2016, 6:26 PM

Jun 16 2016

atrick accepted D21430: Use a much more efficient (linear instead of quadratic?) algorithm for computing the height: standard DFS mincost algorithm for a DAG..

Awesome. Proper DFS is how I've done it in the past.

Jun 16 2016, 9:45 PM
atrick added a comment to D21429: Make the insertion of predicate deps in the schedule graph not quadratic in the number of predicate deps..

TL;DR: The DAG builder is never supposed to form "insane" DAGs.

Jun 16 2016, 5:05 PM
atrick added a comment to D21398: [IVUser] Add a trunc if used only by a single IV user.

Actually, I'm sure fix #1 works in this case, I'm just not sure it won't regress anything else.

Jun 16 2016, 4:32 PM
atrick added a comment to D21398: [IVUser] Add a trunc if used only by a single IV user.

This is definitely a case worth fixing.

Jun 16 2016, 4:31 PM
atrick added a reviewer for D20522: [misched] Extend scheduler to handle unsupported features: MatzeB.

Matthias investigated this very recently, I think he should make a decision on the patch.

Jun 16 2016, 2:55 PM