llvm/test/CodeGen/Mips/ removed here: https://reviews.llvm.org/rG805f51f9fedf90d2aa0ad46c61cb4c9c0c5bcfe9

https://lists.riscv.org/g/sig-toolchains/message/548 has a proposal where X27 is one of the registers.

In D146565#4211989, @MaskRay wrote:

LG! llvm/test/CodeGen/Mips/ tests are unrelated to the driver change and the part should be committed separately.

Removed several testcases related to 'mips.*-linux-android'

Removed testcases.

Thanks for putting the patch.

ping @StephenFan

Platforms like Android reserve a register x18(for AArch64). There was a patch for RISC-V that also reserved x18 for shadow callstack. https://reviews.llvm.org/D14599, https://reviews.llvm.org/D143355

Folded D145979 here

I'll fold this to D145999.

Moved the testcase as suggested by @samitolvanen

Are there possible optimizations which can be implemented to reduce the code-size impact of this change? I'm wondering if this patch adds the checks conservatively to all instances where stack check could potentially, but not necessarily, be missed/useful?

This patch is needed for zbb support as per: https://lore.kernel.org/all/20230117122447.y6tdsmsxqdwf76ri@orel/

In D144927#4162156, @nickdesaulniers wrote:

In D144927#4160250, @efriedma wrote:

If you're going to enable hoisting past a callbr, please add a testcase to ensure we don't hoist a load past a callbr which modifies the memory in question.

Will do. Curiously, GVNHoist::safeToHoistLdSt returns false even for cases of loads not modified by callbr, so HoistGVN doesn't optimize those cases. FWICT, the call to GVNHoist::firstInBB is checking the callbr against itself. It looks like MemorySSA is perhaps claiming that for this input:

@x = global i32 0
@y = global i32 0
define i32 @foo4(i1 %z) {
entry:
  callbr void asm "", "=*m,!i"(ptr elementtype(i32) @x)
          to label %a [label %b]

a:                                                ; preds = %entry
  %0 = load i32, ptr @y, align 4
  ret i32 %0

b:                                                ; preds = %entry
  %1 = load i32, ptr @y, align 4
  ret i32 %1
}

that callbr is a "defining access" for the access of @y (IIUC)? That seems incorrect...but I'm unfamiliar with MemorySSA.

Can this patch also convert this one to strlen? gcc is able to recognize this idiom: https://godbolt.org/z/7ra1ErqKK

@joshua-arch1 i can land this on your behalf if you think this is good to go.

Can we land this patch. I think this is quite useful at -Oz

LGTM, i'll let the code owners make the final call. Thanks for working on this.

this patch is still better than the status quo where it sounds like we're generating incorrect code and might be reasonable stop-gap.

LGTM, thanks for putting this patch.

Also cc: @craig.topper

Thanks for doing this. LGTM, defer to code owners for approval.

Updated the failing testcase.

reopening to update the failing testcase.

thanks for reverting it. i'll take care of the test failure.

@joshua-arch1 can you share your email such that I can commit this with proper author info? Feel free to ping me directly on discord: Aditya Kumar#9439

Link to llvm presentation for reference: https://llvm.org/devmtg/2019-04/slides/TechTalk-Kruppe-Espasa-RISC-V_Vectors_and_LLVM.pdf

cc: @jrtc27 anything you still want addressed?

Is it okay to merge this patch?

if this patch helps make progress for some folks, i think it is reasonable to merge this one as well. @nickdesaulniers do you have any objections to merging this patch?

Any interest in landing it?

LGTM

is it okay to merge this patch or are there pending dependencies?

ping: do we plan to merge this patch?

More changes were needed to make it work on my machine. I've added them in a separate diff: https://reviews.llvm.org/D132523

LGTM

In D132523#3749655, @nickdesaulniers wrote:

Does the description in phab need updating? It suggests multiple invocations of patch?

My understanding is that a basic block with a PHI node should have first instruction as PHI

Make it git compatible.

Added all the changes

Adding more fixes.

nit: please edit the summary with

// (X + C2) | C
// (X + C2) ^ C
// (X + C2) & C

I agree with @eli.friedman that decomposing multiply into two separate instructions may not be profitable.
Even when latency of multiply is high, having two instructions in the pipeline adds complexity (dependencies, re-ordering, retiring etc).

Is there another induction variable optimization pass that this can be merged with?

with arc diff

Not working either. Do I have to reboot the machine/restart a service ?
Machine type:

lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 22.04.1 LTS
Release:	22.04
Codename:	jammy

neat! it'll be great if for loop bounds that don't overflow and can be inferred at compile time, the first version just get DCE'd.

Thanks for enabling this!

In D88460#2816028, @joerg wrote:

Please make sure at the very least that it doesn't get matched if a function named strlen and also test for that.

In D130466#3709163, @gsocshubham wrote:

I have compiled below testcase with options -

../install-aarch64/bin/clang -mthread-model single thread.cpp -S -Ofast
clang-15: error: invalid thread model 'single' in '-mthread-model single' for this target

-------thread.cpp----------

int u, v, restrict, i;

void f(int a[restrict], int b[restrict], int n) {
    for (i = 0; i < n; ++i) {
        if (a[i]) {
            ++u;
            break;
        }
        ++u;
        if (b[i])
            ++v;
    }
}

---

Also, I tried with SPARC -

../install-sparc/bin/clang -mthread-model single thread.cpp -S -Ofast
clang-15: error: invalid thread model 'single' in '-mthread-model single' for this target

while for thread model posix, above commands works fine.

a / 5, and a%10 etc.

Looks pretty interesting. I'm curious if there is a popular workload/benchmark that would improve with this patch?

We usually provide a runtime library for specialized functionalities like this. Are such division operations common enough to benefit from compiler generated code?

FYI, all the tests in this patch are already handled with the current licm. Do we still need this patch?