LGTM after the code change is applied.

Happy to pre-commit the tests if this is something that we actually want to do.

Rebase

Please rebase on top of master to verify this fixes the issue mentioned in D88785.

Landed this, thank you!

Address review commends, rebase

You can remove these notices by running arc diff again with clang-format in your PATH. It is also fine to just leave these messages in – CI will verify the formatting as well and fail if its not correct. Only then it is strictly necessary to rediff your code.

@YangKeao Will you be pursuing this further? Should I take over this for you?

Address review comments, rebase

None from me. As I said I'm not too qualified to have much say in fastisel, so my other comments are broadly ignorable. I do think that some fix for this needs to land (and be nominated for a backport into LLVM 12) sooner rather than later

In D98906#2641418, @andreadb wrote:

Be careful about referencing stack locations below the red-zone.

The x86-64 ABI mandates a 128 bytes red zone. Your new instruction would effectively write a zero at a location below the red-zone.
That region of memory is not reserved, and should be considered volatile. For example: signals and interrupt handlers are allowed to modify it.

Looks like my alternative has caveats of its own. Can you please split your change into two parts? One that affects only the unrolled case (which I believe should be good to land) and the other which affects the loop case (potentially subject to further discussion).

Ah, I see!

Can you please pre-commit the tests so that it is easier to see how the codegen changes? E.g. I suspect the CFI directives in prologue are already broken before your changes and so my comments aren't super relevant.

Done!

rebase

btw I prototyped a D98906: [X86] Improve lowering of the unrolled inline-asm probing yesterday as an alternative approach towards improving the unrolled case.

I investigated this a little bit, and it seems like using createVirtualRegister won't work here, after all. It seems that this code does in fact run after regalloc happens. I'm out of ideas, but I also am not that familiar with this area to give any useful advice or guidance here.

I submitted this as a diff to give an opportunity for people to object. Will land it in 24 hours or so if there aren't any.

I think you may be able to quite easily add a case for S/UMUL_LOHI here as well (but unsure if its necessary, just noticing that its not explicitly handled here)

@RKSimon I cherry-picked your patch on top of mine and there were no changes in test output (as reported by check-llvm), sadly. If anything your differential makes improvements introduced by this differential slightly less impressive.

rebase

See createVirtualRegister.

In D98789#2634751, @YangKeao wrote:

Any register that isn't callee-preserved could be used (or any register in general if its spilled first), assuming it isn't used for something else already. I did a quick search in an effort to figure out which registers are callee-saved on x86, but couldn't find anything definitive :(

It seems that different platform will have different set of callee-preserved register. I also found them in the list in LLVM (which is refered in the function getCalleeSavedRegs of llvm/lib/Target/X86/X86RegisterInfo.cpp). Sadly, it seems like RDI is more likely to be a callee-saved register.

Only considering the SYSV and Windows, all callee-saved registers are:

X86::RBX, X86::R12, X86::R13, X86::R14, X86::R15, X86::RBP
X86::RBX, X86::RBP, X86::RDI, X86::RSI, X86::R12, X86::R13, X86::R14, X86::R15, X86::XMM6, X86::XMM7, X86::XMM8, X86::XMM9, X86::XMM10, X86::XMM11, X86::XMM12, X86::XMM13, X86::XMM14, X86::XMM15

And all available dwarf registers are:

32bit:
EAX, EDX, ECX, EBX, ESI, EDI, EBP, ESP, EIP
64bit, X86-64:
RAX, RDX, RCX, RBX, RSI, RDI, RBP, RSP, R8-R15, RIP

For 64bit, RAX, RCX, RDX, R8-R11 could be a good choice, and for 32 bit, it could only be choosen from EAX, ECX, EDX

Make sense. I tried to use r11 + offset to represent CFA temporarily. However, r11d cannot be used as a dwarf register on x86_32.

I split the NFCs out into a different diff. I also revised the approach slightly in an attempt to reduce code duplication. LMK if that (e.g. P's capture by reference) seems too magical, and I'm happy to revert to duplicating the checks.

rebase onto D98672

backout

Rebase onto D98672

Yeah that looks like the one.

My feeling is that this fix is a work-around for what is a regression llvm11->12. Here's a comment I had sitting incomplete since yesterday in bugzilla:

revert the comments

Rebase onto precommited test cases, adjust style

Adjust the differential name in the test case

Not sure what's up with the other test changes, annotated the POIs with inline comments.

I went through the list of what looked like regressions and attempted to root-cause and validate them. So far there seem to be a few major differences between old/new:

Rebase onto newly added tests for illegal types

Rebase before land

Adjust the RISCV tests as per the comments.

In D98410#2619315, @bjope wrote:

So undef include values that isn't possible for every combination of the other operands.

Wouldn't folding C * undef and undef * C into an undef, rather than 0, be valid/better? Is there a reason we cannot do that?

Split up the test files into srem and urem variants

In D98339#2616612, @RKSimon wrote:

It might be better to split the srem/urem files - its very minor, but it means that all the srem-seteq-* and urem-seteq-* tests group together in file lists

In cases where we do end up with a instruction count regression, the reason appears to be because lowering would fall back to BuildUDIV (and the MULHU/SHIFT strength reduction it implements). The BuildUDIV reduction is slightly shorter in instruction count, but it also depends on the target's ability to cheaply compute the higher half of the multiplication result.

rebase

This seems like a reasonable fix, whatever value my review of fastisel code has.

lint

Pivot to removing the provenance losing transformation

The issue this was trying to solve was effectively resolved by rG2ad1f5eb1a47: [InstCombine] Don't canonicalize (gep i8* X, -(ptrtoint Y)) as (inttoptr…. I'm happy to abandon this, because the change to the other transformation is of questionable correctness anyway.

restore a deleted comment

Rebase

I'm quite surprised this change didn't affect any pre-existing tests. It however, is sufficient to resolve one of a fairly easily hit mis-compiles in Rust.

Just so that it doesn't get lost between hidden comments: I've no commit bit, please commit this for me if this is good to go.

revert the test to its previous form

Check the formatted output from dwarfdump instead