On Feb 11, 2010, at 12:07 PM, David Greene wrote:> On Thursday 11 February 2010 14:02:13 Dan Gohman wrote: > >>>> Putting a bit (or multiple bits) in MachineMemOperand for this >>>> would also make sense. >>> >>> Is there any chance a MachineMemOperand will be shared by multiple >>> instructions? >> >> Yes. > > Then we can't use it to hold a non-temporal flag. The operand might be > non-temporal in one context but it may not be in another.Sharing only happens when two instructions have the "same" memory reference info. You just need to make sure that the non-temporal flag is significant. It's not fundamentally different from the volatile flag in this respect. Dan
On Thursday 11 February 2010 14:44:23 Dan Gohman wrote:> > Then we can't use it to hold a non-temporal flag. The operand might be > > non-temporal in one context but it may not be in another. > > Sharing only happens when two instructions have the "same" memory > reference info. You just need to make sure that the non-temporal > flag is significant. It's not fundamentally different from the > volatile flag in this respect.Ok, this sounds right, but this look wrong: /// Abstact virtual class for operations for memory operations class MemSDNode : public SDNode { [...] bool isVolatile() const { return (SubclassData >> 5) & 1; } Shouldn't that be MMO->isVolatile()? -Dave
While hacking around in the SelectionDAG build code, I've made the
isVolatile, (new) isNonTemporal and Alignment parameters to
SelectionDAG::getLoad/getStore and friends non-default.
I've already caught one bug in the XCore backend by doing this:
if (Offset % 4 == 0) {
// We've managed to infer better alignment information than the load
// already has. Use an aligned load.
return DAG.getLoad(getPointerTy(), dl, Chain, BasePtr, NULL, 4,
false, false, 0);
}
Whoops! There's no alignment info being set here!
Is there any reason to keep these as default arguments? It invites silent
coding errors. I did this because I have to propagate the non-temporal
information through various phases of lowering and making these non-default
helps me catch missing cases.
We've done the same in our local sources and we've never found
specifying the extra arguments to be burdensome.
-Dave
Dan,> Sharing only happens when two instructions have the "same" memory > reference info. You just need to make sure that the non-temporal > flag is significant. It's not fundamentally different from the > volatile flag in this respect.Metadata is used to monitor values from the sideline. It should not influence optimization or code gen in general other then the intended recipient of the metadata info. So your suggestion is not a good idea for all kind of metadata in general. Just a note. - Devang
On Feb 11, 2010, at 3:41 PM, Devang Patel wrote:> Dan, > >> Sharing only happens when two instructions have the "same" memory >> reference info. You just need to make sure that the non-temporal >> flag is significant. It's not fundamentally different from the >> volatile flag in this respect. > > Metadata is used to monitor values from the sideline. It should not > influence optimization or code gen in general other then the intended > recipient of the metadata info. So your suggestion is not a good idea > for all kind of metadata in general. Just a note.I was talking about MachineMemOperands in the above, not metadata. But that does suggest a consideration: if you're using metadata at the LLVM IR level, it may make sense to use metadata at the codegen level too, to avoid this confusion. Dan
On Feb 11, 2010, at 3:15 PM, David Greene wrote:> On Thursday 11 February 2010 14:44:23 Dan Gohman wrote: > >>> Then we can't use it to hold a non-temporal flag. The operand might be >>> non-temporal in one context but it may not be in another. >> >> Sharing only happens when two instructions have the "same" memory >> reference info. You just need to make sure that the non-temporal >> flag is significant. It's not fundamentally different from the >> volatile flag in this respect. > > Ok, this sounds right, but this look wrong: > > /// Abstact virtual class for operations for memory operations > class MemSDNode : public SDNode { > [...] > bool isVolatile() const { return (SubclassData >> 5) & 1; } > > Shouldn't that be MMO->isVolatile()?It's not a bug; the code could be written either way. There's actually an assert in MemSDNode's constructor which checks that they're the same. I believe the code is structured this way because it makes it easy to lump the volatile flag in with other data which is significant for CSE purposes, but it's not critical that it work that way. Dan