[all-commits] [llvm/llvm-project] 788bbd: [DAGCombiner] Improve chain handling in fold (fshl...

[Craig Topper via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 788bbd2ef6a291cc1e38c6d35c3924bf2382e75a
      https://github.com/llvm/llvm-project/commit/788bbd2ef6a291cc1e38c6d35c3924bf2382e75a
  Author: Craig Topper <craig.topper at sifive.com>
  Date:   2025-02-03 (Mon, 03 Feb 2025)

  Changed paths:
    M llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
    M llvm/test/CodeGen/X86/fshl.ll

  Log Message:
  -----------
  [DAGCombiner] Improve chain handling in fold (fshl ld1, ld0, c) -> (ld0[ofs]) combine. (#124871)

Happened to notice some odd things related to chains in this code.

The code calls hasOneUse on LoadSDNode* which will check users
of the data and the chain. I think this was trying to check that
the data had one use so one of the loads would definitely be
removed by the transform. Load chains don't always have users so
our testing may not have noticed that the chains being used would
block the transform.

The code makes all users of ld1's chain use the new load's chain, but
we don't know that ld1 becomes dead. This can cause incorrect dependencies if
ld1's chain is used and it isn't deleted. I think the better thing to do
is use makeEquivalentMemoryOrdering to make all users of ld0 and ld1
depend on the new load and the original loads. If the olds loads become
dead, their chain will be cleaned up later.

I'm having trouble getting a test for any ordering issue with the current code.
areNonVolatileConsecutiveLoads requires the two loads to have the same
input chain. Given that, I don't know how to use one of the load chain
results without also using the other. If they are both used we don't
do the transform because SDNode::hasOneUse will return false for both.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications