MemOperands not updated during ISel argument copy elision, may lead to miscompile #89060
Comments
mikaelholmen
added
bug
|
@llvm/issue-subscribers-bug Author: None (mikaelholmen)
Reproduce with: 06eedff
```
llc bbi-94617_3.ll -o - -debug
```
[bbi-94617_3.ll.gz](https://github.com/llvm/llvm-project/files/15010509/bbi-94617_3.ll.gz)
If we look at the debug printouts, in "Initial selection DAG:" we get Especially consider and The load and store both access "FrameIndex:i64<-1>", but the MemOperands look like and so they access exactly the same memory on the stack, but according to the MemOperands it looks like they access different objects? I think this is wrong. If we look further at the debug printouts at "MI Scheduling" we get So there is no dependency whatsoever between the load, SU(0) and the store SU(3), even if they access the same memory. On my downstream target this then lead to a miscompile when the scheduler choose to change the order of the load and store. I think the problem is that SelectionDAGISel::LowerArguments and tryToElideArgumentCopy does rewrites and change the FI used in some instructions, but they leave the MemOperands unchanged. This seems to be old but it recently lead to a miscompile for my target when the load and store suddenly executed in the wrong order. |
|
One defensive appoach might be to mark the fixed stack object as "isAliased" when doing the argument copy elison. Kind of like this (we also need to implement MachineFrameInfo::setIsAliasedObjectIndex): I think that at least in theory that should make the FixedStackPseudoSourceValue appear as if it aliases with any IR reference. A more advanced solution would need to eliminate all references to the IR variable somehow (e.g. when lowering loads/stores etc) to make sure we only find MachinePointerInfo:s that use the corresponding FixedStackPseudoSourceValue. That seems to be more complicated, and I have no idea if it is worth the trouble. On downstream application C code and benchmarks I could not see any negative impact of the more defensive solution, while it seemed to fix the miscompile found in fuzzy testing. But maybe thing are different for other languages/targets/applications. |
|
Just for reference as I talked about it in the previous comment: |
|
I think this could be a reproducer showing the same bug. Using hexagon as in-tree target: Print after all gives this: So at the end the store |
|
@llvm/issue-subscribers-backend-hexagon Author: None (mikaelholmen)
Reproduce with: 06eedff
```
llc bbi-94617_3.ll -o - -debug
```
[bbi-94617_3.ll.gz](https://github.com/llvm/llvm-project/files/15010509/bbi-94617_3.ll.gz)
If we look at the debug printouts, in "Initial selection DAG:" we get Especially consider and The load and store both access "FrameIndex:i64<-1>", but the MemOperands look like and so they access exactly the same memory on the stack, but according to the MemOperands it looks like they access different objects? I think this is wrong. If we look further at the debug printouts at "MI Scheduling" we get So there is no dependency whatsoever between the load, SU(0) and the store SU(3), even if they access the same memory. On my downstream target this then lead to a miscompile when the scheduler choose to change the order of the load and store. I think the problem is that SelectionDAGISel::LowerArguments and tryToElideArgumentCopy does rewrites and change the FI used in some instructions, but they leave the MemOperands unchanged. This seems to be old but it recently lead to a miscompile for my target when the load and store suddenly executed in the wrong order. |
Adding test case related to llvm#89060 It shows that after argument copy elison the scheduler may reorder a load of the input argument and a store to the same fixed stack entry (the fixed stack entry that is reused for the local variable).
This is a fix for miscompiles reported in llvm#89060 After argument copy elison the IR value for the eliminated alloca is aliasing with the fixed stack object. This patch is making sure that we mark the fixed stack object as being aliased with IR values to avoid that for example schedulers are reordering accesses to the fixed stack object. This could otherwise happen when there is a mix of MemOperands refering the shared fixed stack slow via both the IR value for the elided alloca, and via a fixed stack pseudo source value (as would be the case when lowering the arguments).
Adding test case related to #89060 It shows that after argument copy elison the scheduler may reorder a load of the input argument and a store to the same fixed stack entry (the fixed stack entry that is reused for the local variable).
This is a fix for miscompiles reported in llvm#89060 After argument copy elison the IR value for the eliminated alloca is aliasing with the fixed stack object. This patch is making sure that we mark the fixed stack object as being aliased with IR values to avoid that for example schedulers are reordering accesses to the fixed stack object. This could otherwise happen when there is a mix of MemOperands refering the shared fixed stack slow via both the IR value for the elided alloca, and via a fixed stack pseudo source value (as would be the case when lowering the arguments).
…89712) This is a fix for miscompiles reported in #89060 After argument copy elison the IR value for the eliminated alloca is aliasing with the fixed stack object. This patch is making sure that we mark the fixed stack object as being aliased with IR values to avoid that for example schedulers are reordering accesses to the fixed stack object. This could otherwise happen when there is a mix of MemOperands refering the shared fixed stack slow via both the IR value for the elided alloca, and via a fixed stack pseudo source value (as would be the case when lowering the arguments).
|
Solved by d8b253b |
…lvm#89712) This is a fix for miscompiles reported in llvm#89060 After argument copy elison the IR value for the eliminated alloca is aliasing with the fixed stack object. This patch is making sure that we mark the fixed stack object as being aliased with IR values to avoid that for example schedulers are reordering accesses to the fixed stack object. This could otherwise happen when there is a mix of MemOperands refering the shared fixed stack slow via both the IR value for the elided alloca, and via a fixed stack pseudo source value (as would be the case when lowering the arguments). (cherry picked from commit d8b253b)
…lvm#89712) This is a fix for miscompiles reported in llvm#89060 After argument copy elison the IR value for the eliminated alloca is aliasing with the fixed stack object. This patch is making sure that we mark the fixed stack object as being aliased with IR values to avoid that for example schedulers are reordering accesses to the fixed stack object. This could otherwise happen when there is a mix of MemOperands refering the shared fixed stack slow via both the IR value for the elided alloca, and via a fixed stack pseudo source value (as would be the case when lowering the arguments). (cherry picked from commit d8b253b)
Reproduce with: 06eedff
bbi-94617_3.ll.gz
If we look at the debug printouts, in "Initial selection DAG:" we get
Especially consider
and
The load and store both access "FrameIndex:i64<-1>", but the MemOperands look like
and
so they access exactly the same memory on the stack, but according to the MemOperands it looks like they access different objects? I think this is wrong.
If we look further at the debug printouts at "MI Scheduling" we get
So there is no dependency whatsoever between the load, SU(0) and the store SU(3), even if they access the same memory.
On my downstream target this then lead to a miscompile when the scheduler choose to change the order of the load and store.
I think the problem is that SelectionDAGISel::LowerArguments and tryToElideArgumentCopy does rewrites and change the FI used in some instructions, but they leave the MemOperands unchanged.
This seems to be old but it recently lead to a miscompile for my target when the load and store suddenly executed in the wrong order.
The text was updated successfully, but these errors were encountered: