Page MenuHomePhabricator

add getAA in ScheduleDAG for using in HazardRec
Needs ReviewPublic

Authored by atheel.ma on May 12 2018, 10:44 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

atheel.ma created this revision.May 12 2018, 10:44 PM

add getAA in ScheduleDAGMI for using in HazardRec
add getAA in SchedulePostRATDListfor using in HazardRec

atheel.ma edited reviewers, added: sunfish; removed: dblaikie.May 12 2018, 11:24 PM
atheel.ma retitled this revision from add getAA in ScheduleDAGMI for using in HazardRec to add getAA in ScheduleDAGM for using in HazardRec.
atheel.ma retitled this revision from add getAA in ScheduleDAGM for using in HazardRec to add getAA in ScheduleDAG for using in HazardRec.
atrick accepted this revision.May 13 2018, 7:35 PM

LGTM

This revision is now accepted and ready to land.May 13 2018, 7:35 PM

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

comment for nullptr return added
added function getAA() to the base class also for cases that you have dynamic pointer of base class ScheduleDAGInstrs that holds SchedulePostRATDList

in SchedulePostRATDList it cant be nullptr because its a pass that intilize the AA in his run

I dont have commit permissions:

Permission to llvm-mirror/llvm.git denied to atheelm

can you push it or tell how to get permissions?

thanks,
Atheel

atrick resigned from this revision.May 15 2018, 10:07 AM
atrick added a reviewer: hfinkel.

Atheel,

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

Once you have contributed a couple sizable patches, you can email Chris Lattner to get commit access.

I could commit this for you, but I just noticed something strange going on. There are two pointers to the current AliasAnalysis, one in ScheduleDAGInstrs and one in ScheduleDAGMI. It looks like they may be set under different conditions. I'm afraid combining them under the same getAA() virtual method may just create more confusion. It looks like Hal was last to work on this.

Hal, can you take a look and commit this in whatever form makes sense to you?

This revision now requires review to proceed.May 15 2018, 10:07 AM

Atheel,

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

Once you have contributed a couple sizable patches, you can email Chris Lattner to get commit access.

I could commit this for you, but I just noticed something strange going on. There are two pointers to the current AliasAnalysis, one in ScheduleDAGInstrs and one in ScheduleDAGMI. It looks like they may be set under different conditions. I'm afraid combining them under the same getAA() virtual method may just create more confusion. It looks like Hal was last to work on this.

Hal, can you take a look and commit this in whatever form makes sense to you?

There's an AAForDep because some targets don't want the cost of doing AA queries during instruction scheduling (or no one feels like tracking down the performance regressions we'd see from enabling it (as would be the case on X86 last we looked at this)).

How would this be used?

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

Atheel wants to use AliasAnalysis within the hazard recognizer. I think it's important to avoid adding more implementation classes to headers that will be included/subclassed by out-of-tree targets. But getting the AA pointer from the existing API seems reasonable.

From that discussion:

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:

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 the same as above for PostRASchedule pass.

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…

thanks Atrick for the explanation,

I do use PostRASchedulerList , anything wrong about it?

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

Atheel wants to use AliasAnalysis within the hazard recognizer. I think it's important to avoid adding more implementation classes to headers that will be included/subclassed by out-of-tree targets. But getting the AA pointer from the existing API seems reasonable.

Sure, makes sense to me.

From that discussion:

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:

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 the same as above for PostRASchedule pass.

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 Hfinkel, are you going to push it?