[llvm] [MachineLICM] Rematerialize instructions that may be hoisted before LICM (PR #158479)

[via llvm-commits]

dianqk wrote:

> @dianqk following up on my previous comment ([#158479 (comment)](https://github.com/llvm/llvm-project/pull/158479#issuecomment-3289693202)) In your specific testcase from the aforementioned issue, it seems like those hoisted adds had two defs:
> 
>     * one explicit def: dst register
> 
>     * one implicit def: eflags register
> 
<details><summary>...</summary>
<p>

> It seems that in this case, the eflags dst def of said add insn isn't actually used by anything Which makes me wonder if it'd be better to open a new PR which patches the `if (I.getNumDefs() > 1)` for the AggressiveCycleSink code, to, in the case where `I.getNumDefs()` > 1:
> 
>     * check if there's only one _explicit_ def and if so, if all implicit defs are just on `$eflags` (early exit if either condition is false)
> 
>     * treat instruction `I` as only having one def (meaning, proceed to sink instruction `I`) if the implicit ~`$eflags` def isn't actually used by any instruction proceeding instruction `I`~ def(s) is/are marked dead
> 
> 
> That should allow `-mllvm -sink-insts-to-avoid-spills=1` to handle _most_ instructions hoisted by the middle end licm (and that should still handle most selects, since most selects should be lowered to a `test` or `cmp` x86 insn followed by `cmovCC`, since the `test` and `cmp` insns should only have an `eflags` def)
> 
> I would imagine that the only otherwise sinkable insns that would still otherwise be able to be handled by `AggressiveCycleSink` would be arithmetic insns where the eflags def is not used by any `JCC`, but is used by one or more `cmovCC`/`setCC`/`sbb`/etc, likely in the same BB the arithmetic insn is in. I would reckon that the easiest to handle would be arithmetic insn + `setCC`, whereas w/ `cmovCC` & `sbb` would require you to additionally consider the src def(s) of the `cmovCC`/`sbb` insn(s).
> 
> Either way, I definitely think it'd be best to first create a PR that just handles the simpler case of arithmetic insns w/ one explicit def & one `$eflags` def, whose output `$eflags` def isn't used by any following insn. **EDIT**: I just realized: the aforementioned hoisted add insns' implicit `$eflag` def are already marked as `implicit-def dead $eflags` and according to the MIR register flags documentation: https://llvm.org/docs/MIRLangRef.html#register-flags `implicit-def dead` means that the implicit def is marked as **unused** so improving the heuristic of `AggressiveCycleSink` should be simpler than I originally thought **EDIT 2**: simple patch that should hopefully achieve the same result as @dianqk's draft PR: [main...sharkautarch:llvm-project:MIRAggrSink_ignoreDeadDefs](https://github.com/llvm/llvm-project/compare/main...sharkautarch:llvm-project:MIRAggrSink_ignoreDeadDefs) **EDIT 3**: seems like my above patch leads to an assert fail in one specific case: `ld.lld: /tmp/makepkg/llvm-git/src/llvm-project/llvm/lib/CodeGen/InlineSpiller.cpp:1012: bool (anonymous namespace)::InlineSpiller::foldMemoryOperand(ArrayRef<std::pair<MachineInstr *, unsigned int>>, MachineInstr *): Assertion 'MO->isDead() && "Cannot fold physreg def"' failed.`
> 
> Seems to be caused by my patch making MachineSink sink a MOV32r0 @RKSimon any idea of how to check for MOV32r0 without explicitly checking for that specific opcode? Hmm… my one idea would be that, since MOV32r0 is printed as `%gr:<dst> = MOV32r0 implicit-def dead $eflags`, which makes it seem like MOV32r0 doesn’t internally hold a reg use maybe I can just avoid sinking MOV32r0 by not sinking any instructions that have more than one def and no reg uses…
> 
> **EDIT 4**: Not sinking multi-def insns that don't have any reg uses works to prevent the assert from happening


</p>
</details> 

It sounds fine, but `SinkInstsIntoCycle` is false by default. We need a way that can work with the default options.

https://github.com/llvm/llvm-project/pull/158479