[AMDGPU] stop image_store being moved illegally
ClosedPublic

Authored by tpr on Thu, Jan 11, 3:19 PM.

Details

Summary

A recent change
321556: AMDGPU: Remove mayLoad/hasSideEffects from MIMG stores
can allow the machine instruction scheduler to move an image store past
an image load using the same descriptor.

As a temporary workaround until we figure out what is going wrong with
alias analysis, this commit makes all image load and store instructions
volatile.

Diff Detail

Repository
rL LLVM
tpr created this revision.Thu, Jan 11, 3:19 PM

I think changing AMDGPUImagePseudoSourceValue::isAliased() to return true may fix this.

I think changing AMDGPUImagePseudoSourceValue::isAliased() to return true may fix this.

This is the most conservative way to fix it. I think the better way to fix this would be to change AMDGPUImagePseudoSourceValue::mayAlias() to true. You could improve this even further by making sure that image instructions that reference the same resource have the same PseudoSourceValue. ScheduleDAGInstrs::buildSchedGraph() is where all the dependencies are calculated.

tpr updated this revision to Diff 129596.Fri, Jan 12, 3:18 AM

V2: The more conservtive of Tom's suggested fixes.

tpr added a comment.Fri, Jan 12, 3:19 AM

Thanks Tom. Just setting mayAlias() didn't fix it. So I have gone with setting isAliased() as well. We should revisit this, but maybe it's not worth it until we get round to doing fat pointers.

tpr updated this revision to Diff 129625.Fri, Jan 12, 7:35 AM

V3: Reverted test change done on 321556.

arsenm accepted this revision.Fri, Jan 12, 8:21 AM

I think changing AMDGPUImagePseudoSourceValue::isAliased() to return true may fix this.

This is the most conservative way to fix it. I think the better way to fix this would be to change AMDGPUImagePseudoSourceValue::mayAlias() to true. You could improve this even further by making sure that image instructions that reference the same resource have the same PseudoSourceValue. ScheduleDAGInstrs::buildSchedGraph() is where all the dependencies are calculated.

That images have the same resource argument do have the same PSV now. I think the issue may be the non-constant offsets in the intrinsics aren't represented anywhere

This revision is now accepted and ready to land.Fri, Jan 12, 8:21 AM

I think changing AMDGPUImagePseudoSourceValue::isAliased() to return true may fix this.

This is the most conservative way to fix it. I think the better way to fix this would be to change AMDGPUImagePseudoSourceValue::mayAlias() to true. You could improve this even further by making sure that image instructions that reference the same resource have the same PseudoSourceValue. ScheduleDAGInstrs::buildSchedGraph() is where all the dependencies are calculated.

That images have the same resource argument do have the same PSV now. I think the issue may be the non-constant offsets in the intrinsics aren't represented anywhere

Ok, it looks like maybe just setting the MachineMemOperand size to something non-zero may work for now.

tstellar accepted this revision.Fri, Jan 12, 8:37 AM

As is, this patch is fine. Other improvements can be done as follow on work.

This revision was automatically updated to reflect the committed changes.