assigned to @anton-afanasyev

Why is this an issue for AArch64? Only (32-bit) M-class and R-class CPUs can trap on division by zero.

Why is this an issue for AArch64? Only (32-bit) M-class and R-class CPUs can
trap on division by zero.

X86 (didn't check other targets) for example mark SDIV/UDIV as hasSideEffect and it prevent the optimization/bug.

I'm asking why this is a bug at all on AArch64 (the target in your llc invocation). I could see it being a problem on some ARMv7, possibly also resolved by marking division as having side-effects.

I am not familiar with AArch64 target, we only use it as reproducer to show the problem ( we have out of tree target) .

I believe adding MCID::UnmodeledSideEffects flag to arm DIV instr would be a correct fix of this bug. Ordinary instructions (like add or shift) still could be correctly hoisted in such a way for the given CFG.

I think that's what I'm leaning towards too. It's a shame "mayRaiseFPException" is named so specifically because it feels even more apt otherwise.

I don't think that UnmodeledSideEffects is the solution, it limit Scheduler a lot.
I also think that in-order HW will get performance
degradation due to https://reviews.llvm.org/rL362901. Is it code size or performance optimization ?

I don't think that UnmodeledSideEffects is the solution, it limit Scheduler a lot.

I'm sympathetic, but unless you can come up with a way it is modeled there's not really any alternative. It clearly has some undesirable side-effect.

Though the more I look at mayRaiseFPException, the more appropriate it looks for this use-case except for the name. In principle we can even arrange the "FPExcept" flag to be set correctly based on the architecture in ARM.

I don't think that UnmodeledSideEffects is the solution, it limit Scheduler
a lot.
I also think that in-order HW will get performance
degradation due to https://reviews.llvm.org/rL362901. Is it code size or
performance optimization ?

It's performance optimization primarily, since it eliminates partial redundancy of computation.

I can also add special condition to prevent hoisting for such CFG (not so strong as suggested here https://reviews.llvm.org/rL362901#663831 though) since it makes no performance effect (only code size one). This would be a workaround for CSE step possibly not being able to recognize regs pressure causing by elimination, but it is an other issue, not related to the original bug (though it fixes it).

The original bug should be fixed by something like "mayRaiseException" flag.

IMHO, such an optimization, which adds a speculative execution of an operation, especially when done in Machine instruction level, should be very careful when hoisting an operation as a speculative execute.
This at least requires a dedicated flag like "canSpeculate" or "mayRaiseException" as stated by Anton.
Marking such an instruction with hasSideEffect is too aggressive and will result in an unjustified performance damage.
Or at least, to enable a target hook check that would exclude specific machine instructions from being candidates.

Btw, I've made a modification to PRE condition preventing it from useless hoisting: https://reviews.llvm.org/D63934. This change fixes this bug implicitly but I still believe the actual solution should involve checking special flag like "mayRaiseFPException".

FWIW, the failure I'm seeing in Halide due to https://reviews.llvm.org/rL362901 is not resolved https://reviews.llvm.org/D63934.

Hi Alina, could you provide me with test case or at least its control flow graph? We can separate your case from this issue.

Yes, I am working on it.

(Moving discussion back to this thread from
https://reviews.llvm.org/D63934)

Ping! Hi Alina, Ayman, Igor, I've asked before (https://reviews.llvm.org/D63934#1592654) whether the new patch from Kai Luo (https://reviews.llvm.org/D64394) fixes your particular issues. I can revert my original patch if it is not the case.

Hi Anton,

Apologies for the delayed reply. We no longer see the original failure, but it appears it is not due to changes in your original patch, that triggered the failure. The possible source of the fix may be in the HVX backend.
Please rely on the other folks' input on whether to revert r362901.

Hi Anton,

Apologies for the delayed reply.
The Kai Luo patch doesn't fix the issue.
The alternative to revert it to let to Subtarget to decide if speculative execution allowed and if PerformSimplePRE optimization should/can be done.

Hi all, I've benchmarked the effect of the revertion my and @​lkail patches.
The benchmark showed some increase of the exec_time (details are here https://reviews.llvm.org/D56772#1623312).
So I'd like to leave these changes unreverted and to proceed with particular patches. Please give LGTM to this patch: https://reviews.llvm.org/D63934. I'm to prepare the next patch for Subtarget allowance of speculative execution.
Please tell if you have any objections.

Hi Anton,

Apologies for the delayed reply.
The Kai Luo patch doesn't fix the issue.
The alternative to revert it to let to Subtarget to decide if speculative
execution allowed and if PerformSimplePRE optimization should/can be done.

Hi Igor,

Does this patch https://reviews.llvm.org/D66132 is what you need to fix the issue? You can set isSpeculativeExecutionForbidden() to true for your particular Subtarget. Alternative way is to set hasSideEffect() to particular instructions.

Hi Anton,

Apologies for the delayed reply.
The Kai Luo patch doesn't fix the issue.
The alternative to revert it to let to Subtarget to decide if speculative
execution allowed and if PerformSimplePRE optimization should/can be done.

Hi Igor,

Does this patch https://reviews.llvm.org/D66132 is what you need to fix the
issue? You can set isSpeculativeExecutionForbidden() to true for your
particular Subtarget. Alternative way is to set hasSideEffect() to
particular instructions.

Yes, Thank you!

Hi,

Is this fixed? I think it is - https://www.diffchecker.com/SvD3hVXM/.