Page MenuHomePhabricator

[SelectionDAGBuilder] Don't set MachinePointerInfo for gather when we find a uniform base
ClosedPublic

Authored by craig.topper on Fri, Mar 13, 1:27 PM.

Details

Summary

I believe we were previously calculating a pointer info with the scalar base and an offset of 0. But that's not really where the gather is pointing. The offset is a function of the indices of the GEP we looked through.

Also get rid of the constant memory stuff for gather because I'm not sure that was right either.

I'm also not sure the memory size for the gather/scatter is correct either since I think it using the vector VT, but we're really loading/storing multiple disjoint scalars.

Diff Detail

Event Timeline

craig.topper created this revision.Fri, Mar 13, 1:27 PM

We want to try to create the best MachineMemOperand we can. That means:

  1. In general, we can't express the semantics in MachinePointerInfo; the Value has to be a pointer, not a vector of pointers. So we have to create an "empty" MachinePointerInfo. (But we should make sure to note the address-space correctly.)
  2. If we find a uniform base, we should be able to express the notion that each loaded value has to be "based on" that pointer. I'm not sure we can just pass it to the MachinePointerInfo; probably we need some way to express that the offsets could be negative. But it shouldn't be a big extension. Even if the access offset/size is unknown, we can still figure out some useful aliasing information. (Two pointers to distinct objects can't alias.)
  3. The size of the access should reflect the actual distance between the accesses, not the number of bytes accessed. If we don't have any way to express an access of unknown size, we should add it (it would be useful in other contexts). Maybe -1ULL just works?

I'm not sure what you think is wrong with the pointsToConstantMemory check. If a gather loads from constant memory, it obviously can't alias anything.


We should be able to write testcases for all of this, I think; there are DAGCombines that depend on whether we can prove aliasing between a load and a store.

We want to try to create the best MachineMemOperand we can. That means:

  1. In general, we can't express the semantics in MachinePointerInfo; the Value has to be a pointer, not a vector of pointers. So we have to create an "empty" MachinePointerInfo. (But we should make sure to note the address-space correctly.)

Ok I'll fix the address space.

  1. If we find a uniform base, we should be able to express the notion that each loaded value has to be "based on" that pointer. I'm not sure we can just pass it to the MachinePointerInfo; probably we need some way to express that the offsets could be negative. But it shouldn't be a big extension. Even if the access offset/size is unknown, we can still figure out some useful aliasing information. (Two pointers to distinct objects can't alias.)
  2. The size of the access should reflect the actual distance between the accesses, not the number of bytes accessed. If we don't have any way to express an access of unknown size, we should add it (it would be useful in other contexts). Maybe -1ULL just works?

I suspect the type legalization code for splitting gathers/scatters is also miscalculating the size. It just calls getStoreSize() on the split VT pieces.


I'm not sure what you think is wrong with the pointsToConstantMemory check. If a gather loads from constant memory, it obviously can't alias anything.

I wasn't sure if passing just the "base" pointer was correct.


We should be able to write testcases for all of this, I think; there are DAGCombines that depend on whether we can prove aliasing between a load and a store.

Do any of those DAG combines even consider gather/scatter nodes or do they just look at loadsdnode/storesdnode?

Do any of those DAG combines even consider gather/scatter nodes or do they just look at loadsdnode/storesdnode?

Oh, hmm, maybe we don't actually try to use aliasing to improve chains past a gather/scatter at the moment. I thought we just had some generic code for MemSDNodes, but I looked and I'm not seeing it.

I wasn't sure if passing just the "base" pointer was correct.

It doesn't really matter what the MachineMemOperand looks like for an operation that doesn't alias anything. :)

I wasn't sure if passing just the "base" pointer was correct.

It doesn't really matter what the MachineMemOperand looks like for an operation that doesn't alias anything. :)

Sorry not the MachineMemOperand, the MemoryLocation we use to call AA->pointsToConstantMemory is using the uniform base ignoring the GEP indices. Is that safe?

I don't think pointsToConstantMemory actually pays attention to the offset/size at the moment, so it's probably safe in practice. Theoretically, I guess we're passing the wrong size/offset, yes.

Preserve address space and change size to UnknownSize.

Herald added a project: Restricted Project. · View Herald TranscriptSun, Mar 15, 11:39 PM
This revision is now accepted and ready to land.Mon, Mar 16, 11:48 AM
This revision was automatically updated to reflect the committed changes.