[PATCH] D89447: [MachineInstr] Add support for instructions with multiple memory operands.

[Michael Liao via Phabricator via llvm-commits]

hliao added a comment.

In D89447#2334424 <https://reviews.llvm.org/D89447#2334424>, @jonpa wrote:

> In D89447#2332645 <https://reviews.llvm.org/D89447#2332645>, @uweigand wrote:
>
>> @jonpa can you check whether the SystemZ test case you added still checks what it was intended to check here?
>
> I actually see a problem with this patch: The SystemZ test case passes the original test by not producing any scalar load + vector element insertions instructions. However now I see that all PFD (prefetch) instructions end up last in the block, whereas before they were not. This may or not be good if the block is huge and the prefetching is therefore not done too many iterations ahead. Maybe this should be checked with benchmarks to make sure the current prefetching tuning does not loose by this.
>
> The reason seems to be that the SystemZ::PFD instruction is marked both with mayLoad and mayStore, but now this patch looks at the *memory operand* and figures out that it is only loading and therefore does not alias with another load. This check was previously only done with the MI flags. The post-RA scheduler now puts all of them at the end.

Even if I removed the following check, the result is still the same

  

The real reason is that prefetch is iterations ahead and won't alias to any memory access in that tloop body. Previously, we

In D89447#2336415 <https://reviews.llvm.org/D89447#2336415>, @hliao wrote:

> Remove the MMO non-store check.



In D89447#2334424 <https://reviews.llvm.org/D89447#2334424>, @jonpa wrote:

> In D89447#2332645 <https://reviews.llvm.org/D89447#2332645>, @uweigand wrote:
>
>> @jonpa can you check whether the SystemZ test case you added still checks what it was intended to check here?
>
> I actually see a problem with this patch: The SystemZ test case passes the original test by not producing any scalar load + vector element insertions instructions. However now I see that all PFD (prefetch) instructions end up last in the block, whereas before they were not. This may or not be good if the block is huge and the prefetching is therefore not done too many iterations ahead. Maybe this should be checked with benchmarks to make sure the current prefetching tuning does not loose by this.
>
> The reason seems to be that the SystemZ::PFD instruction is marked both with mayLoad and mayStore, but now this patch looks at the *memory operand* and figures out that it is only loading and therefore does not alias with another load. This check was previously only done with the MI flags. The post-RA scheduler now puts all of them at the end.

I removed that check that one of those two MMOs needs to be store. No change to test now. But, that sounds a little weird that one instruction claims mayLoad and mayStore but only has isLoad MMO. Maybe we need to change that MMO in prefetch to be both isLoad and isStore.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89447/new/

https://reviews.llvm.org/D89447