Page MenuHomePhabricator

Add TargetCustom type to PseudoSourceValue
AbandonedPublic

Authored by kariddi on Sep 16 2015, 5:44 PM.

Details

Summary

PseudoSourceValue can be used to attach a target specific value for "well behaved" side-effects lowered from target specific intrinsics.
This is useful whenever there is not an LLVM IR Value around when representing such "well behaved" side-effected operations in backends by attaching a MachineMemOperand with a custom PseudoSourceValue as this makes the scheduler not treating them as "GlobalMemoryObjects" which triggers a logic that makes the operation act like a barrier in the Schedule DAG.

This patch adds another Kind to the PseudoSourceValue object which is "TargetCustom". It indicates a type of PseudoSourceValue that has a target specific meaning (aka. LLVM shouldn't assume any specific usage for such a PSV)

Diff Detail

Event Timeline

kariddi updated this revision to Diff 34952.Sep 16 2015, 5:44 PM
kariddi retitled this revision from to Add TargetCustom type to PseudoSourceValue.
kariddi updated this object.
kariddi added a subscriber: llvm-commits.
kariddi updated this revision to Diff 34954.Sep 16 2015, 5:55 PM

Updated MIRPrinter to handle this new enum val in switch case.

kariddi planned changes to this revision.Sep 16 2015, 6:17 PM

Mmm, actually I think this will interact funnily with MIR as this is not a "generic" backend concept, but is very target specific.

Should something like this even being serializable/parsable to/from MIR?

arsenm added a subscriber: arsenm.Sep 16 2015, 6:24 PM

Could you extend this to accept any greater value than the max as TargetCustomN in case a target wants to distinguish multiple different kinds of values?

if you want tests for this, I think AMDGPU could use this for texture operations which currently don't have set MMOs resulting in the scheduler problem.

Do you mean change "isTargetCustom()" to something like (not final):

unsigned isTargetCustom() const {

unsigned KindVal = (unsigned) Kind;
return (KindVal >= TargetCustom) ? ((KindVal+1) - TargetCustom) : 0;

}

such that it returns 1, 2, 3 ... if the Kind is bigger or equal to TargetCustom and 0 if it is not a TargetCustom?

Do you mean change "isTargetCustom()" to something like (not final):

unsigned isTargetCustom() const {

unsigned KindVal = (unsigned) Kind;
return (KindVal >= TargetCustom) ? ((KindVal+1) - TargetCustom) : 0;

}

such that it returns 1, 2, 3 ... if the Kind is bigger or equal to TargetCustom and 0 if it is not a TargetCustom?

Yes, exactly

if you want tests for this, I think AMDGPU could use this for texture operations which currently don't have set MMOs resulting in the scheduler problem.

Yeah, that would be great.
This is the kind of use from where this idea comes from.

kariddi updated this revision to Diff 34958.Sep 16 2015, 6:41 PM
  • Updated to implement Matt's suggestion.
  • Speculatively modified the MIR printer to not handle the TargetCustom PSV as it should be lower than the level MIR printer/parser operates .... but I'm not sure of that and I need a look from somebody working on MIR as I don't know a lot about it.
qcolombet edited edge metadata.Sep 17 2015, 9:50 AM

Hi Marcello,

Add Alex and or Duncan as reviewers. They are the ones that know about the MIR serialization.

Cheers,
-Quentin

Done that, thanks Quentin!

Marcello

arsenm added inline comments.Sep 17 2015, 12:03 PM
include/llvm/CodeGen/PseudoSourceValue.h
67

Since this returns the Kind ID value instead of a bool, a name like getTargetCustom would be better

kariddi marked an inline comment as done.Sep 17 2015, 5:50 PM
kariddi added inline comments.
include/llvm/CodeGen/PseudoSourceValue.h
67

Agreed , done in the next patch

kariddi updated this revision to Diff 35051.Sep 17 2015, 5:51 PM
kariddi marked an inline comment as done.

Still waiting for comments about the MIR serialization stuff.

pete added a subscriber: pete.Sep 17 2015, 6:15 PM

I might be mis-remembering, but I thought we were trying to remove PSV’s?

Only commit I could see which remotely suggested that might be the case is r206255 where the comment contained

"Anything that needs to use either a PseudoSourceValue* and Value* is strongly encouraged to use a MachinePointerInfo instead.”

I’ve CCed Nick to see if he can remember whether there was more work to come on PSV’s or whether they are here to stay. If they are going to be removed in future then its something to think about here before adding more code which relies on them.

Pete

kariddi added a comment.EditedSep 17 2015, 8:27 PM

Thanks Pete for your comment!
I think the commit message you mentioned means that , because Value and PseudoSourceValue are not anymore in the same class heirarchy, trying to use them in a way that requires both an LLVM Value and a PseudoSourceValue at the same time makes it "impossible
Basically I think that the word "either" in the commit message should actually have been "both".

That said if the same can be achieved without passing a PseudoSourceValue to the MachinePointerInfo that would be great even if probably it would provide a little bit less flexibility in the capability of specifying target specific inforrmation used by later passes as the MachinePointerInfo alone doesn't have any field that can be used to store custom data. (Maybe the data can be somehow packed in the offset, but doesn't seem very flexible and is a little bit hacky).

I tried removing the attachment of the PseudoSourceValue to the MachineMemOperand and actually the scheduler continues to not treat the node as a scheduling barrier as if the PSV was attached , so from that point of view it is fine using a MachinePointerInfo without a value attached. It still makes the ScheduleDAG builder happy!

I'm interested though in hearing what is the intended destiny of PSVs (or their intended usage).
Thanks for CCing whoever might know more about it!

kariddi abandoned this revision.Oct 8 2015, 2:47 PM

Resubmtting with full context