LGTM

LGTM

In D58088#1405089, @glenjofe wrote:

That looks simple enough. I haven't gone through the spec but something as simple as:

namespace llvm {
template<class Ptr>
auto to_address(const Ptr &P) noexcept { return P.operator->(); }
template<class T>
constexpr T* to_address(T* P) noexcept { return P; }
}

seems to cover the majority of it (and definitely the bits I need). It's just the std::pointer_traits<Ptr>::to_address(p) based implementation of the fancy pointer overload that's missing.

If it is named to_address and intended to convert any pointer to a raw pointer then it should be:

namespace llvm {
template<class T>
constexpr T* to_address(T* p) noexcept { return p; }
template<class Ptr>
typename std::pointer_traits<Ptr>::element_type* to_address(const Ptr &p) noexcept { return llvm::to_address(p.operator->()); }
}

In D58088#1404872, @dblaikie wrote:

In D58088#1404769, @dsanders wrote:

I just noticed a big simplification that doesn't need C++20 features or new classes. This:

template <typename WrappedIteratorT,
          typename T1 = typename std::remove_reference<decltype(**std::declval<WrappedIteratorT>())>::type,
          typename T2 = typename std::add_pointer<T1>::type>
using raw_pointer_iterator = pointer_iterator<pointee_iterator<WrappedIteratorT, T1>, T2>;

is functionally equivalent to the raw_pointer_iterator in this patch. pointee_iterator derefs the unique_ptr, while pointer_iterator produces the raw_pointer. If that version sounds good to you then I think it makes sense to revert the llvm::to_address() patch, switch to this implementation and fold this into the GISel Combiner patch as at that point it's too trivial to warrant a dedicated patch. Does that sound good?

Beyond probably some annoying compile-time complexity/overhead (& maybe similar debugging confusion with the extra layers of abstraction) - I'm guessing this wouldn't support null pointers (because pointee_iterator probably dereferences unconditionally?) which would be a bit problematic, I'd reckon?

I just noticed a big simplification that doesn't need C++20 features or new classes. This:

template <typename WrappedIteratorT,
          typename T1 = typename std::remove_reference<decltype(**std::declval<WrappedIteratorT>())>::type,
          typename T2 = typename std::add_pointer<T1>::type>
using raw_pointer_iterator = pointer_iterator<pointee_iterator<WrappedIteratorT, T1>, T2>;

is functionally equivalent to the raw_pointer_iterator in this patch. pointee_iterator derefs the unique_ptr, while pointer_iterator produces the raw_pointer. If that version sounds good to you then I think it makes sense to revert the llvm::to_address() patch, switch to this implementation and fold this into the GISel Combiner patch as at that point it's too trivial to warrant a dedicated patch. Does that sound good?

Late bound return, remove noexcept, and update comments to match

The std::to_address implementation is in https://reviews.llvm.org/D58421. I had to add another limitation to it as it accidentally used C++14 which prevents the deduction of the return type.

In D58088#1401583, @dblaikie wrote:

In D58088#1396951, @dsanders wrote:

In D58088#1395226, @paquette wrote:

Hmm, It might be nice to have this as a specialization of pointer_iterator if possible.

I think it might be a source of confusion/frustration to have both a raw_pointer_iterator and a pointer_iterator, where raw_pointer_iterator is specifically used for only unique_ptr.

I can see a way to achieve that (std::to_address()) but it requires C++20. Without that, the closest I've been able to get is a raw_pointer_iterator alias to pointer_iterator that has specializations based on whether a third optional argument is true_type or false_type. The main thing I'm missing to fold the alias in is a way to specify the default value of T in a way that resolves to either pointer_iterator's default type T or raw_pointer_iterators default type T depending on whether *Iter is a std::unique_ptr<X>& or not. I can detect the type easily enough but selecting between the two defaults is proving troublesome as it resolves the true and false cases too early and ends up trying to do int::pointer and failing because int isn't a class type.

Would it be difficult to have LLVM's own version of std::to_address (LLVM has had/does have various type traits implmented in-tree when they aren't available in the standard LLVM's using at the moment) or similar to use here?

In D52954#1397449, @bogner wrote:

Is there a more generic term for this than "signpost"? If so, it might be nice to use that to avoid confusion when/if other implementations show up. If not, that's fine.

Updated before commit

In D58232#1399984, @ab wrote:

On second thought, what happens to the tablegen emitter? That needs to be taught about these as well, right? That might not trigger on anything right now though.

LGTM

In D58088#1395226, @paquette wrote:

Hmm, It might be nice to have this as a specialization of pointer_iterator if possible.

I think it might be a source of confusion/frustration to have both a raw_pointer_iterator and a pointer_iterator, where raw_pointer_iterator is specifically used for only unique_ptr.

ping

LGTM. Thanks for fixing this

In D57449#1389860, @arsenm wrote:

I'm not sure why the MMO alignment is uint64_t in the first place. The maximum supported alignment is 1 << 29 (which I guess when converted to bits is 1 too many for uint32)

In D57449#1388378, @aemerson wrote:

@dsanders does this look ok to you?

In D57243#1385543, @qcolombet wrote:

The legalizer is already done the way I'm talking about on the rule matching side and performance was marginally better on average than the old setAction() interface.

Got it.
That said, to me the legalizer is no different than ISel or a sort of InstCombine, thus I would expect we eventually use the same engine/automaton for selecting/lowering what goes through. The same goes with the way we represent them, although admittedly legalization is a bit more complicated because we have to come up with a way of representing the constraints on the unbound input types.

In D57243#1385507, @qcolombet wrote:

Personally, I'd like to replace the LegalizationAction with a function pointer that just performs the desired change and have a standard library of actions that can be called for the common changes.

@dsanders didn't we try something like that for ISel and the performance was terrible?

In D57484#1383417, @arsenm wrote:

In D57484#1383409, @paquette wrote:

In D57484#1383408, @arsenm wrote:

In D57484#1383395, @paquette wrote:

@arsenm, adding this opcode breaks AMDGPU somehow. Do you have any idea why that might be?

Breaks what?

Here's an example:
http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/24100/steps/build%20stage%201/logs/stdio

Some tablegen seems unhappy?

I have no idea. I haven't looked at the DAG compatibility stuff

In D57029#1373957, @aemerson wrote:

In what situations can these kinds of loads be generated?

At least for AArch64, the extending loads combiner only runs before legalisation, so it wouldn't have a chance to combine these. In the absence of a G_ANYEXTLOAD opcode, I think having an explicit G_ZEXTLOAD is preferable here even though it's slightly pessimistic.

@dsanders thoughts?

In D55914#1364207, @tstellar wrote:

You can actually use iPTR types in patterns already, but the GlobalISel backend converts it to a scalar integer when generating the ISel code.

LGTM

LGTM

LGTM

I'm not sure this is a significant problem at the moment (I'll find out the next time I'm debugging :-)), but just to mention it. One unfortunate consequence of this is presumably that no output is written to a file when tblgen crashes because the output was only written to memory and that was lost in the crash. FWIW, being able to see the successfully-produced output has been fairly handy for me in the past as it helps to establish the context which I can then use to minimize the input or determine a strategy for debugging it. This can be particularly helpful when lldb fails to print important variables (which seems to happen often for me), and the same stack trace occurs frequently during execution. We might want to consider an option that makes it write directly to a file if that does become a problem.

LGTM

Rebase and fix the remaining issue about documenting the right-to-modify
MachineInstrs within the specified function and making the non-const ptr
scheduling have consistent requirements with the const ptr scheduling.

LGTM. The only comment I have is an observation that odd-length vectors generally don't fit well into fewerElements at the moment but that's not for this patch