[PATCH] D135451: [TTI] New PPC target hook enableUncondDivisionSpeculation

Tue Oct 11 08:27:43 PDT 2022

nlopes added a comment.

In D135451#3849630 <https://reviews.llvm.org/D135451#3849630>, @bmahjour wrote:

> In D135451#3844084 <https://reviews.llvm.org/D135451#3844084>, @efriedma wrote:
>
>>> As I understand it integer divide by zero is considered undefined behaviour in C while the same may not be true in other languages. Furthermore presence of side effects may be dependent on other configurations including target hardware (eg default treatment on Power vs x86).
>>
>> The LLVM IR rule is more driven by the behavior of the instructions on various targets. If a target only has a trapping divide, we'd need to wrap it in control flow to implement a non-trapping divide.  And particularly for signed divide, that check isn't cheap. We tend to prefer poison where it makes sense (for example, out-of-bounds shifts).
>>
>> Frontends can always use control flow to get whatever user-visible behavior they want.  (For example, the Rust divide operator panics if you divide by zero.)
>>
>>> To allow more flexibility we could either leave the IR neutral and let the optimizer decide based on config info (eg. TTI)
>>
>> We try to avoid making core IR semantics depend on TTI.  Not that we can completely ignore target differences when writing IR optimizations, but we want to keep IR understandable without reference to target-specific semantics.
>>
>> I mean, it would be self-consistent to write in LangRef something like "whether division by zero is undefined behavior, or defined to produce a poison value, depends on the current target/current subtarget/bitwith of the operation/current moon cycle".  But I don't want to go there.  If the rules are the same across all targets, it's easier to understand, and easier to implement tools like Alive2 to validate transforms.
>
> I'm sorry but I don't quite follow the logic that tries to justify the current design. On the one hand the IR rules are driven by target requirements, and at the same time you avoid making IR semantics dependent on TTI (which is the proper way to query about target requirements). They seem like recipes for the exact problem we are dealing with here, ie there are target-specific assumptions baked into the IR that are not configurable.

The design is influenced by the all the ISAs we want to support, which means it won't be optimal for some (or any), but that's life. That doesn't imply the semantics of the IR must be parameterizable on TTI information.

Changing something fundamental like the semantics of the division instruction has far reaching effects. Even if we wanted to do it, your patch is very incomplete. We would need to audit every single optimization/analysis that touches divisions and make sure they would still be correct with your proposed semantics. Removing UB from division (as you propose) makes some optimizations wrong as they can't assume that RHS is always non-zero, for example.

In summary, changing semantics of div is not something we are willing to do. The risk is too high and the benefits seem low. The best way forward for you is to investigate existing intrinsics or create a new one.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135451/new/

https://reviews.llvm.org/D135451