In D88385#2501562, @rengolin wrote:

In D88385#2498908, @jrtc27 wrote:

Yeah my concern is not about m68k doing weird things, it's about the code that's added in the target-independent places to support m68k that I am most concerned with, as it affects every target regardless of whether m68k is enabled.

Exactly, that's why I prefer to keep the CodeBeads implementation for now (that only affects the m68k target) than refactor table-gen (that would affect all).

We should avoid mixing generic refactory with experimental code, unless the refactory is a clear benefit to all targets, not just the experimental one.

I concur with @RKSimon that we should approve this code as is and get the target in.

In D88385#2498889, @glaubitz wrote:

In D88385#2498795, @RKSimon wrote:

FWIW I still think we're better off keeping (m68k only) CodeBeads rather than refactoring a lot of code that will affect mainstream targets. After m68k has been pushed, we can reinvestigate whether to keep CodeBeads (and whether other targets would benefit from using it), or moving m68k to a refactored TSFlag mechanism - we can make either a pre-requisite for it losing its experimental status if necessary.

I very much agree with that stance and I think this is the most straight-forward approach. I think getting the backend merged so becomes visible to a broader audience - both testers and developers - will help improve the quality and squeeze out more bugs. After all, it's supposed to be marked as experimental so it's expected to not be production-ready yet.

Which GDB tests? How are they broken? I'm extremely surprised by the given this patch implements what binutils has done for a very long time (only .asciz/.string have changed behaviour in binutils, and those are not modified by this patch here so should not have regressions).

Please make sure there are tests for invalid instructions (especially checking you have the immediate ranges and predicates correct).

Having said that I do think it makes sense to push the getOS call into getOSABI by passing the full triple as it simplifies all the callers (and that also decouples the NFC refactor from your proposed behavioural change).

Looking at the original motivation for the patch I'm not convinced that llvm-readelf etc shouldn't just fall back on displaying the "normal" set of GNU extensions even for ELFOSABI_NONE (i.e. those that FreeBSD and GNU have in common). Things like that are really just part of de-facto generic ELF these days. What does binutils do?

In D94345#2488047, @LemonBoy wrote:

The test is not so solid (is it?), ideally we should check the MI to make sure the load ops have the correct offset but I have no idea of what kind of test does that.

🎉 (looks sensible to me but don't have a huge amount of experience with MIR)

One thing I don't understand is why RVV incoming/outgoing arguments need their own stack slots. I assume they're just passed indirectly, so why do we need to distinguish between arguments and locals rather than just treat them like anything else that ends up being passed indirectly?

Your tests look like copies of the F/D/Zfh tests with not all the comments updated and instances of tests that just don't make sense for Zfinx. I only skimmed them and picked up a few issues, I haven't gone through them thoroughly, please do that yourself.

In D88385#2476359, @Paul-C-Anagnostopoulos wrote:

I was thinking of using a bit in the other flags member, Flags, as opposed to TSFlags. Then there would be nothing sneaky going on in the union itself.

Making it an anonymous union would mean that all current references to TSFlags would still work, correct?

In D88385#2476329, @Paul-C-Anagnostopoulos wrote:

@jrtc27 Will some sort of union of a uint64_t and a pointer work? One of the Flags bits could specify which one it is.

I agree that it would be cleaner to encode the information as flags, if possible.

In D88385#2476326, @myhsu wrote:

In D88385#2476220, @Paul-C-Anagnostopoulos wrote:

If it isn't possible to encode the information in TSFlags, another possibility is to add a pointer to the MCInstrDesc class that points to some kind of auxiliary class/struct.

This actually gives me an idea to directly use the MCInstrDesc::TSFlags as a pointer to auxiliary data structures. That is:

auto* AuxTable =  reinterpret_cast<uint8_t*>(MD.TSFlags);

I still don't understand what problem code beads are trying to solve that isn't already solved by existing backends like X86. Why can't you just assign operands to instruction encoding bits like a normal backend?

In D52050#2466874, @glaubitz wrote:

In D52050#2441164, @glaubitz wrote:

In D52050#2441141, @jrtc27 wrote:

What gets done currently for i386? That suffers potentially the same problem given both /usr/lib/gcc/x86_64-linux-gnu/$V/32 and /usr/lib/gcc/i386-linux-gnu/$V are possible locations for an i386 GCC toolchain.

Very good suggestion. I will look into that tomorrow. Thanks for the pointer!

I have been wrapping my head around this for some time and I have run into one problem trying to apply the suggested approach.

The problem is that I don't know how to tell whether I'm on an x86_64 system or an x32 system there is no `case llvm::Triple::x32:` which would be needed here (we have it for x86).

x32 takes the switch case for x86_64, so x32 and x86_64 targeting x32 are identical.

However, that's not the same as whether we're on an x86_64 system or on an x32 system determines which GNU triplet to use and which include and library search paths are our primary ones.

I agree with @lenary, I think this is fine to land (with the whitespace fix :)) and if it turns out the threshold is too low then we'll soon find out and can adjust accordingly (but I imagine not, 5 feels like a sensible default). Or if it's too high!

In D93624#2465904, @sepavloff wrote:

In D93624#2465776, @jrtc27 wrote:

Doesn't this render fesetround useless?

No. fesetround sets dynamic rounding mode (register frm), which may be any mode. However in the case of non-default rounding mode (all other than RNE), floating operations must be represented by constrained intrinsics, for example llvm.experimental.constrained.fadd should be used instead of fadd.

Doesn't this render fesetround useless?

I see very little by way of testing the return side of the calling convention, and nothing for sret.

Looks good to me, let's see what the others think.

Firstly, please generate your diffs with full context (-U with a sufficiently-large number). Secondly, can we avoid having to do a bunch of duplication with some clever use of multiclasses for F/D/Zfh and pseudos? Though maybe it's small enough that the duplication is easier to reason about than an obfuscated abstracted version.

i.e. can we not just support both approaches and prefer x86_64-linux-gnux32 if it exists?

In D52050#2441133, @glaubitz wrote:

In D52050#2441094, @hvdijk wrote:

I've been able to check what Ubuntu 20.10 offers in terms of x32 support. Its kernel supports x32 binaries, it provides x32 versions of core system libraries in separate packages (e.g. libc6-x32, libx32stdc++6, libx32z1), and it provides a compiler that targets x32 by default (gcc-x86-64-linux-gnux32).

I did that, too. In fact, Ubuntu is identical to Debian in this regard as both the Ubuntu and the Debian gcc packages are maintained by the same maintainer (Matthias Klose whom I also happen to know personally) who first uploads these packages to Debian unstable, then syncs to Ubuntu.

However:

These Ubuntu packages do not use the Debian/Ubuntu multiarch approach: the packages are completely independent of the corresponding x64 and i386 versions with separate names, and nothing in Ubuntu installs any libraries into any /lib/x86_64-linux-gnux32-like path. They are intended to allow x32 cross compilation on an Ubuntu system, not intended to act as a basis for running an x32 Ubuntu system. This appears to be very different from Debian's x32 support. That said, cross-compiled binaries do run on Ubuntu and it should be possible to build an x32-native LLVM with the Ubuntu-provided toolchain.

Well, Debian has both as - as I already mentioned - the gcc packages in Debian and Ubuntu are the same. The only difference is that Ubuntu does not provide an x32 port so there is no possibility to install Ubuntu x32 packages in the commonly known MultiArch manner.

Since MultiArch and the -cross packages are somewhat redundant, I'm not so sure yet which approach to address this issue would be best. I will talk to Matthias regarding this.

In D92841#2440962, @lenary wrote:

@jrtc27 it sounds like adding @plt into all the tests won't break anything, and will catch if future changes default to non-plt calls, so this change is an improvement?

In D92841#2439763, @lenary wrote:

The spills look right to me. The question we're not sure of is the appearance of the @plt suffixes on all the external calls.

Note that this shows there is currently another bug in x86_64 clang (ignoring OS, since with manual -march=athlon64 on macOS you can also get the non-macOS behaviour). Whether or not libatomic is used is part of the ABI, as libatomic may (and likely will for 16B atomics on your standard Linux distro) use hash tables of locks to emulate atomic operations, which will not correctly synchronise with the inlined hardware atomics. See https://godbolt.org/z/fqfvev for evidence of the ABI breakage.

Ah, I hadn't realised the F comment got lost in 741b04b0b7912611a8a5b7e74462e87b8930a116, I was looking at our fork which is currently still based on 11. This looks good to me now.

Yes that looks like everything now; thanks!

In D52050#2428743, @hvdijk wrote:

In D52050#2428735, @glaubitz wrote:

Hmm, I was pretty sure that autoconf can deal with x32 inside an x32 chroot.

Most autoconf-using software won't be dealing with a copy of config.guess that was last updated in 2011. Updating that to a more recent version should also work. :)

It's also wrong if the ADDI has FrameReg as its destination. A neater approach might be to always use the destination register (if there is one, using general instruction query information) as the destination for the ADD, keeping the scratch register for the movImm call. Then you'd only need special logic to work out whether you need to keep the (modified) existing instruction.

This is a bit ugly. Surely a simple peephole optimisation should be able to fix this and any other cases more generally? But also please give this a better title and commit message.

I thought I'd gone through it thoroughly last time but apparently not :( oh well hopefully the last set of nits.

Seems good other than additional comments regarding code clarity.

In D71124#2423633, @oceanfish81 wrote:

@khchen ,
how should I obtain available (v)CPU features?
Any sample code, that should work on RISC-V?

Couple of minor changes but otherwise LGTM.

In D92269#2421399, @Paul-C-Anagnostopoulos wrote:

Yes, we could create !codeconcat instead, along with !codeinterleave, !codeeq, etc. We could also just extend the existing bang operators to work on the code type. I thought it was cleaner just to get rid of the distinction.

Losing types and permitting nonsensical operators like !subst on code seems a bit sad. Why not just define a !codeconcat, like how we have a separate !listconcat? I agree that the TypeOf_xxx stuff is really ugly and so having _greater_ expressivity like !codeconcat would go a long way to help that. Also, cast<code>(str) would be the natural way to express the cases where you still really do want to construct code as a string without needing TypeOf_xxx.

This seems fine to me now other than the outstanding discussion about code size.

You still appear to have the old patch that doesn't change addDisp.

Currently P0-P1 is valid and results in the program address space being 1, but this patch changes the semantics of that. How sure are you that nothing will break? I do not like the idea of reusing existing valid syntax to mean something else; if you want to introduce some notion of secondary program address spaces then the syntax should be different.

My intuition is that there should be tests for the various instructions that are only hard-float ABIs, and tests specifically for all the different calling conventions that use a very limited set of instructions.

In D91717#2411334, @edward-jones wrote:

In D91717#2407884, @luismarques wrote:

It seems a bit excessive to me to coalesce the entry points into bundles of 4. Do you have any particular benchmarking data or reasoning that supports choosing that threshold?
Also, shouldn't this implementation include CFI directives?

I used bundles of 4 just to follow the behaviour I saw in libgcc, and the grouping of 2 for rv64 seemed a bit too fine-grained. I'm not sure what the original justification for the coalescing into groups of 2/4 was in libgcc.

I'll update to account for other suggested changes and see if I can find any benchmarks which show the tradeoff for the grouping threshold