[PATCH] D147968: [TTI][BPF]: Undo specific transform-preventing passes and add one TTI hook

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D147968#4416082 <https://reviews.llvm.org/D147968#4416082>, @wenlei wrote:

> I think that in general honoring canonicalization is good, but it's not always a clear cut. Legality and profitability are sometimes relative, and in the case of BPF, I do think there's case to be made for mid-end to look at TTI more then what is traditionally allowed. The failure mode here is different from profitability, where you may just end up with inefficient code if you don't undo later; for BPF though, it can generate program that will be rejected by verifier (not run slower). Technically BPF backend can define what is legal for that target, i.e. one could argue that BPF target requires preserving predicates in certain form (needs to be well defined though), which would then restrict certain optimization from the mid-end.

I am, generally speaking, not opposed to doing TTI-based legality checks in special circumstances. I have approved exceptions for doing so in the past. A recent one was using TTI in InstCombine to check whether a certain address space cast is legal for the target. Without TTI, we assume that all address space casts are illegal. This exception made sense to me, because addrspacecast semantics are fundamentally target-dependent, and the legality check is compatible with the LangRef semantics.

The case here is very different, because the legality conditions for BPF are not well-defined and not compatible with IR semantics. BPF considers transforms "illegal" that are clearly legal under our operational semantics, and, to the best of my knowledge, there is no principled way a maintainer could determine whether or not a given transform would be "legal" for BPF or not.

We have other targets with somewhat "unusual" legality requirements, where the target does not accept arbitrary inputs. These include things like wasm (which has verifier requirements) and gpu targets like amdgpu and nvptx (which also have verifier requirements, as well as convergence restrictions). These additional legality requirements are handled in one of two ways: Either the backend takes the responsibility of converting IR into a legal form (e.g. CFG structurization or removal of irreducible cycles) or the additional legality requirements are encoded into the IR, e.g. through the use special intrinsics, operand bundles and/or token values. This makes the legality constraint part of the normal IR semantics, and we can use our normal reasoning and tools to determine whether transforms are legal or not.

I maintain my position that the BPF target should be using either of those approaches (or a combination of them). Otherwise we will end up with checks for the BPF target littered over random places in the code base, with a new check being added every time a commit breaks the BPF verifier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147968