In D99605#2679568, @Paul-C-Anagnostopoulos wrote:

I pushed this patch.

In D99605#2677896, @Paul-C-Anagnostopoulos wrote:

Do you think it's necessary to have the change of document title reviewed in Phabricator?

In D99009#2666793, @thakis wrote:

Looks like this doesn't build on windows: http://45.33.8.238/win/36271/step_4.txt

Please take a look and revert for now if it takes a while to fix.

Thanks, and good point re ordering

In D98936#2661530, @sepavloff wrote:

In D98936#2661518, @jrtc27 wrote:

In D98936#2661504, @sepavloff wrote:

In D98936#2661232, @jrtc27 wrote:

In D98936#2661228, @asb wrote:

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

If you want to have different scheduling for different CSRs, do you not need per-CSR pseudos in order to express that? This diff is ostensibly to allow for that in future, but is at a much coarser read/write/swap granularity, so doesn't really get you much over and above just scheduling CSRRW itself as a whole, just adds more complexity for little gain. IMO any CSRs we need scheduling info for should just get their own dedicated read/write/swap pseudos as and when they're needed.

The patch D99083 demonstrates the solution for FP state/control resisters. Every register and every access get separate pseudos, each of which can have their own scheduling properties.

That looks like the kind of thing I'm imagining. So can we remove the Read/Write/Swap_CSR defs from this diff and just keep the Read/Write/SwapSysReg classes for use with such pseudos?

No problem. I just thought that putting Read/Write/Swap_CSR defs (which are general purpose definitions) into a patch specific for FP support is not quite logical. But if this is OK, I'll move them.

In D98936#2661504, @sepavloff wrote:

In D98936#2661232, @jrtc27 wrote:

In D98936#2661228, @asb wrote:

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

If you want to have different scheduling for different CSRs, do you not need per-CSR pseudos in order to express that? This diff is ostensibly to allow for that in future, but is at a much coarser read/write/swap granularity, so doesn't really get you much over and above just scheduling CSRRW itself as a whole, just adds more complexity for little gain. IMO any CSRs we need scheduling info for should just get their own dedicated read/write/swap pseudos as and when they're needed.

The patch D99083 demonstrates the solution for FP state/control resisters. Every register and every access get separate pseudos, each of which can have their own scheduling properties.

In D98936#2661228, @asb wrote:

In D98936#2642039, @jrtc27 wrote:

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

Hi @jrtc27 - could you please elaborate a little on the concern that the approach in this path might be the worst of both worlds? I'm not sure I fully follow. Thanks.

The fact that this is under NDEBUG at all seems rather dubious

Most likely because you're adding assembly tests in Clang, which won't work if the backend isn't present (and needs a REQUIRES line). Assembly tests in Clang are generally bad practice and best avoided for that reason. If you do want them it's probably best to split them out into their own tests so people can still run the frontend tests regardless.

Are there ever any cases where you _wouldn't_ want a CSR-specific pseudo in order to have control over the scheduling of it specifically? This feels a bit like a middle-ground that's the worst of both worlds to me.

In D99026#2639788, @craig.topper wrote:

In D99026#2639787, @jrtc27 wrote:

I think the final line of your commit message is the wrong way round?

Maybe it was poorly written. How does it look now?

I think the final line of your commit message is the wrong way round?

Please fix the style issues and update the patch with full context (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface, or use arc)

I think these are just for debug output? Though maybe they appear in MIR too?

CSR addresses are uimm12s not simm12s

This should really be for all OSes on RISC-V, which I think means copying it N times? :/

In D98113#2634758, @luismarques wrote:

I just looked at this again and I don't have the full context in my mind right now but won't the test just exercise the BareMetal toolchain and not your changes?

I don't know why it only affects microMIPS (presumably it does its lowering subtly differently from normal MIPS) but those changes look sensible to me

In D98616#2626094, @jrtc27 wrote:

In D98616#2626093, @kito-cheng wrote:

GCC use vr for vector register and vm for vector mask register.

How does that even work? Aren't multi character strings a set of options?

In D98616#2626093, @kito-cheng wrote:

GCC use vr for vector register and vm for vector mask register.

This seems like the obvious choice for the constraint, but it would be good to ensure there's consensus with GCC people, especially since their assembly constraints are intimately tied to their instruction patterns (or, really, the assembly constraints just expose those and we pretend to be GCC) so they have less flexibility than us.

In D98542#2623036, @craig.topper wrote:

Improve comment and try to improve function name to encapsulate new behavior.

Hm, this seems like a bit of an abuse of what the function is intended to do, at least with its current name and comment, as it's now not just doing normalisation for mapping to the set of supported instructions but also optimising at the same time.

In D98449#2622341, @craig.topper wrote:

In D98449#2621957, @jrtc27 wrote:

This feels like it should be a target-independent DAGCombine?

DAG combine and instcombine canonicalize to constants on the RHS and SETGT, SETLT, SETUGT, and SETULT. So X <= 0 will always be rewritten to X< 1.

This feels like it should be a target-independent DAGCombine?

Any TableGen backend error that's a real message and not an assertion failure with a backtrace is a win in my books :)

In D98113#2610775, @abidh wrote:

In D98113#2609027, @jrtc27 wrote:

One thing that is currently rather ugly about BareMetal is that it ignores the AddArch argument. Once you have a sysroot, the architecture suffix is rather unnecessary, and given compiler-rt uses LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to decide both whether to add a suffix and whether to add lib/${OS} to the install path, that means you can't nicely point compiler-rt at the sysroot as the install directory (well, unless you do something like make COMPILER_RT_OUTPUT_DIR point at the sysroot not the libdir and set COMPILER_RT_OS_DIR to . (or maybe the empty string also works)).

Agree that setting the compiler-rt installation directory is not ideal. I remember I ended up doing some post installation steps to make sure that libraries were in right folder.

Regarding this patch, currently Baremetal toolchain looks for compiler-rt in a multilib specific directory. Does that not solve the problem of finding compiler-rt for any given arch/abi combination automatically?

In D88391#2576777, @RKSimon wrote:

@jrtc27 Do you have any more feedback before I accept this and we let the experimental backend be pushed to trunk?

Hm, having SHA-2 acceleration but not SHA-3 in 2021 is a bit surprising to me

One thing that is currently rather ugly about BareMetal is that it ignores the AddArch argument. Once you have a sysroot, the architecture suffix is rather unnecessary, and given compiler-rt uses LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to decide both whether to add a suffix and whether to add lib/${OS} to the install path, that means you can't nicely point compiler-rt at the sysroot as the install directory (well, unless you do something like make COMPILER_RT_OUTPUT_DIR point at the sysroot not the libdir and set COMPILER_RT_OS_DIR to . (or maybe the empty string also works)).

... and another accidental change

Dropped unintended change

Added missing file for test

In D94163#2605594, @sepavloff wrote:

In D94163#2603738, @asb wrote:

We discussed this briefly in the RISC-V call as I noted this patchset has been sat open for some time. One thing that might be helpful is whether you could say a little bit more about the goal for this patchset.

Most floating point instructions set accrued exception bits in fflags register. If an instruction is specified with dynamic rounding mode, it also depends on the content of frm register. So in the following code:

csrwi  frm, a1
fadd.d ft2, ft2, ft3

changing the order of instruction is not allowed, because fadd.d depends on the content of frm, which is changed by the previous instruction. Similarly, the code:

fadd.d ft2, ft2, ft3
csrrs t0, fflags, zero

does not allow to change the order of the instructions, as crsrs reads content of fflags, which is set by the first instruction.

Now nothing prevents the compiler from changing the order of instructions in these examples. Existing instruction definitions are unable to express these dependencies. It does not allow to write programs that use non-default floating point environment, for example, dynamic rounding mode.

The goal of this patchset is to establish means to express such dependencies.

Once this lands, what's the next step?

This is the first and necessary step toward full-fledged implementation of floating point support. In particular it would allow progress in D91242 and D90854. Actually this work was undertaken because the lack of dependencies prevents implementation of llvm.set.rounding (https://reviews.llvm.org/D91242#2400476). The next steps, of course, include support of constrained intrinsics for RSCV.

Is there some relevant RFC,

If RFC can facilitate review of this patchset, I will prepare it.

or equivalent changes being made to other in-tree architectures?

Targets that support full-fledged floating point operations add implicit uses and definitions of FP state and control register(s). For example:
• X86: https://github.com/llvm/llvm-project/blob/bc172e532a89754d47fef1306064a26a4dc0a76b/llvm/lib/Target/X86/X86InstrFPStack.td#L728 (see let Defs = [FPSW] and let Uses = [FPCW]),
• PowerPC: https://github.com/llvm/llvm-project/blob/e7361c8eccb7663146096622549dc03240414157/llvm/lib/Target/PowerPC/PPCInstrInfo.td#L3169 (see Uses = [RM]),
• SystemZ: https://github.com/llvm/llvm-project/blob/9e28b89827a3be4ab602b40c263839665af06b4a/llvm/lib/Target/SystemZ/SystemZInstrFP.td#L434 (see let Uses = [FPC])

We're also still unclear about the advantage of changing codegen to default to a static rounding mode (which might be a surprising change, as all software compiled to date on both GCC and LLVM has used used the dynamic rounding mode by default).

Instructions in assembler without explicit rounding mode specification get dynamic rounding mode as now. Lowering of FP operations like fadd uses static rounding mode RNE, because these operations assume default floating point environment (https://llvm.org/docs/LangRef.html#floating-point-environment). Using static rounding mode has some advantages over assuming frm to have particular value. The code that requires default rounding mode does not require setting rfm in a program where some pieces uses non-default rounding mode. Such code works as designed even if it is called from a region where other rounding mode is set. Such implementation simplifies implementation of things like #pragma STDC FENV_ROUND and make programs more robust.

Still a lot of instances of bool isFoo variables/arguments that should be bool IsFoo.

Hopefully the last set of comments for this patch

Is there any way I can usefully test this? As far as I can tell there's only a single constexpr test in the tree that uses the new interpreter, and it's pretty trivial?

In D64146#2591830, @jrtc27 wrote:

In D64146#2591732, @jrtc27 wrote:

In D64146#2567710, @nand wrote:

CodePtr points into the bytecode emitted by the byte code compiler. In some instances, pointers to auxiliary data structures are embedded into the byte code, such as functions or AST nodes which contain information relevant to the execution of the instruction.

Would it help if instead of encoding pointers, the byte code encoded some integers mapped to the original objects?

I've read through the code and have slightly more understanding now. It seems there are several options:

1. Keep the pointers somewhere on the side and put an integer in the byte code, like you suggest

2. Pad values in the byte code to their natural alignment in general (and ensure the underlying std::vector<char> gets its storage allocated at an aligned boundary / use a different container), though this can get a little weird as the amount of padding between consecutive arguments varies depending on where you are (unless you force realignment to the max alignment at the start of a new opcode)

3. Make the byte code be an array of uintptr_t instead of packing it like is done currently, with care needed on ILP32; that can either just use uint64_t instead and we declare CHERI unsupported for 32-bit architectures (which is unlikely to be a problem as you probably want a 64-bit virtual address space if the doubling pointer size, with 64-bit CHERI capabilities on 32-bit VA systems being only for embedded use) or you can split 64-bit integers into two 32-bit integers and treat them as two arguments

1 works but feels ugly. 2 or 3 would be my preference, and mirror how "normal" interpreters work, though those might split the code and data so they can keep the opcodes as, say, 32-bit integers, but the stack full of native word/pointer slots; my inclination is that 3 is the best option as it looks like the simplest. How do you feel about each of those? Is memory overhead from not packing values a concern?

Hm, though I see the "store an ID" pattern is common for dynamic things and this should be quite rare, so maybe that is indeed the right approach, mirroring something like getOrCreateGlobal?

Reworked code slightly to make it look nicer after clang-format uglified it

In D64146#2591732, @jrtc27 wrote:

In D64146#2567710, @nand wrote:

CodePtr points into the bytecode emitted by the byte code compiler. In some instances, pointers to auxiliary data structures are embedded into the byte code, such as functions or AST nodes which contain information relevant to the execution of the instruction.

Would it help if instead of encoding pointers, the byte code encoded some integers mapped to the original objects?

I've read through the code and have slightly more understanding now. It seems there are several options:

1. Keep the pointers somewhere on the side and put an integer in the byte code, like you suggest

2. Pad values in the byte code to their natural alignment in general (and ensure the underlying std::vector<char> gets its storage allocated at an aligned boundary / use a different container), though this can get a little weird as the amount of padding between consecutive arguments varies depending on where you are (unless you force realignment to the max alignment at the start of a new opcode)

3. Make the byte code be an array of uintptr_t instead of packing it like is done currently, with care needed on ILP32; that can either just use uint64_t instead and we declare CHERI unsupported for 32-bit architectures (which is unlikely to be a problem as you probably want a 64-bit virtual address space if the doubling pointer size, with 64-bit CHERI capabilities on 32-bit VA systems being only for embedded use) or you can split 64-bit integers into two 32-bit integers and treat them as two arguments

1 works but feels ugly. 2 or 3 would be my preference, and mirror how "normal" interpreters work, though those might split the code and data so they can keep the opcodes as, say, 32-bit integers, but the stack full of native word/pointer slots; my inclination is that 3 is the best option as it looks like the simplest. How do you feel about each of those? Is memory overhead from not packing values a concern?

In D64146#2567710, @nand wrote:

CodePtr points into the bytecode emitted by the byte code compiler. In some instances, pointers to auxiliary data structures are embedded into the byte code, such as functions or AST nodes which contain information relevant to the execution of the instruction.

Would it help if instead of encoding pointers, the byte code encoded some integers mapped to the original objects?