Any progress on a fix?

The IMPLICIT_DEF is erased only if there some "incoming" value from "other reg".

Why would we need to keep this impdef?

Note: A possible fix could be:

diff --git a/llvm/lib/CodeGen/SplitKit.cpp b/llvm/lib/CodeGen/SplitKit.cpp
index 4029c855c910..a1029bcbc71c 100644
--- a/llvm/lib/CodeGen/SplitKit.cpp
+++ b/llvm/lib/CodeGen/SplitKit.cpp
@@ -655,13 +655,20 @@ VNInfo *SplitEditor::defFromParent(unsigned RegIdx,
         if (S.liveAt(UseIdx))
           LaneMask |= S.LaneMask;
       }
-      assert(LaneMask.any() && "Interval has no live subranges");
     } else {
       LaneMask = LaneBitmask::getAll();
     }

It does improve the original by considering the COPY from the same subreg.

The comment seems outdated, if my understanding is right, and even the original code cannot perform that change since, once the 2nd COPY with same source is found in L1407, the check @ L1419 just skips that earlier as the 1st COPY has no subreg and the 2nd COPY has sub1.

Ping!

I must be missing something, but it feels to me that this patch is actually making the situation worse.

Weird, I added case test/CodeGen/PowerPC/sink-down-more-instructions-1.mir. Could you do another check?

1: add one more mir test case

LGTM

As expected, the improvement for the benchmark is gone now. : (

• Fix a few call sites where I was passing TTI instead of the boolean for UseInstrInfo (it doesn't help that the compiler didn't warm on these)

• Remove const_cast that was a left over from the proof of concept

Gentle ping @nikic.

The real world case I met is not a loop invariant instruction, it has an operand which is a user of PHI in loop header.

I had a (slightly) closer look and I have a question:

• Use Register default constructor instead of forcing = 0

The update with the RegClassWeight is only partially done IMHO.
Each weight should be added/removed from the related register pressure sets. You can take a look at MachineLICM.cpp to see how to use these.

Abandon in favor of D87342.

Clean up the proof of concept:

• Add the proper includes
• Fixup clang-format acting up

Ping! @arsenm

Sorry for the late reply I missed your update.

• Use auto for MachineIRBuilder instead of use the register directly

• Fix typo in comment (G_EXT -> G_ZEXT)

Hi Amara,

• Use TargetTransformInfoImplCRTPBase instead of BasicTTIImpl to avoid requiring the CodeGen library for the unit tests

I've made a proof of concept in D87342 on how we could have some pieces of computeKnownBits be target dependent.

The bulk of the changes are plumbing TTI into the computeKnownBits APIs. On top of that, clang-format did its magic and there are a lot of changes.

Thanks for the comments.

Hi Jessica,

• Add G_FCONSTANT support

• Remove getImm(), this is illegal for G_CONSTANT

• Update AMDGPU tests

• Use setIntrAndDebugLoc

LGTM with one caveat, do we need the new getOneDefInstr?

LGTM, nitpicks below.

Let me know if the current compile time impact is acceptable or if I need to come up with a different strategy.

One thing we could do to help compile time is maybe add more caching to the Query API.
Right now it only caches llvm.assume AFAICT, whereas we could cache known bits for values. That's what we did in the Machine IR variant of computeKnownBits.

Does this (and follow-up patches) improve any public/private benchmarks? Any data?

• Add KnownBits::sextOrTrunc

Thanks for the updated numbers!

I've made the suggested changes, how do I kick a new compile time measurement?

Use AddrKnownBits.isUnknown instead of having a dedicated bool variable.

Possibly adding the early exit on unknown bits will help.

Thanks for the review.

• Rework the logic so that TrailZ and AddrKnownBits share most of the logic
• Fix the handling of structure accesses in the process.

Add scalable vector support (well really abort on scalable vector types!)

Let me see if I can massage the IR to work around this issue.

Note: I also have one failure with that patch: Analysis/ScalarEvolution/scalable-vector.ll
warning: Compiler has made implicit assumption that TypeSize is not scalable. This may or may not lead to broken code.

Can your add also IR testcase from that PR? (To show that Instcombine can fold it)

Use GEPOperator instead of GEPInstr in cast. GEP can come form constant expression as well.

Honestly, I have no idea one way or another. I'm leaning toward @arsenm's theory that the implicit def may be needed in case the full register is never fully covered.
But I would need to dig deeper into this.

I am a bit confused as by what is expected by RA in this case.

LGTM

Regardless of what happens to INLINEASM_BR, I think we still need a terminator copy

Thanks Bill for the detailed explanations. A couple more questions.

I've scanned through the patch and it looks sensible.

I don't think introducing the TCOPY opcode is a valid solution.
Let's assume the copy doesn't get coalesced, that means it needs to be executed after the inlineasm branch. And this is not going to happen if it sits in the same basic block right after the jump.