In D126085#3540577, @reames wrote:

Address test check comment.

Anything remaining blocking this? I know I need to file the psABI issue and will do so before pushing. Asking about further review items.

As just discussed in the sync-up call meeting, let's defer the question of setting / not setting the attribute to an upstream discussion in riscv-elf-psabi-doc on the precise semantics of the tag. Philip kindly volunteered to kick off an issue in that repo.

This is all that's needed to hook up the appropriate attribute:

diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
index 5f9ed77d07cf..ac0c8113135a 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp
@@ -50,6 +50,9 @@ void RISCVTargetStreamer::emitTargetAttributes(const MCSubtargetInfo &STI) {
   else
     emitAttribute(RISCVAttrs::STACK_ALIGN, RISCVAttrs::ALIGN_16);

In D126085#3538325, @reames wrote:

In D126085#3538293, @asb wrote:

I don't think it affects this patch (as splitting scalar vs vector features for alignment makes sense regardless of the frontend option), but I had a quick look at the frontend option for this. Looks like on the GCC side it's -mno-strict-align. On Arm/AArch64 it's spelled -munaligned-access and __ARM_FEATURE_UNALIGNED is set. Unless there's some target generic #define that's set that I'm missing, it feels like it would be useful to set a define for the RISC-V case as well (and agree this with the GNU folks).

I agree this is separate. What a target defaults to should not be blocked on what a frontend might chose to override. I'll note that these flags don't appear to influence x86 handling of the corresponding cases, so unless this is handled somewhere generically I missed, this isn't RISCV specific in any way.

I don't think it affects this patch (as splitting scalar vs vector features for alignment makes sense regardless of the frontend option), but I had a quick look at the frontend option for this. Looks like on the GCC side it's -mno-strict-align. On Arm/AArch64 it's spelled -munaligned-access and __ARM_FEATURE_UNALIGNED is set. Unless there's some target generic #define that's set that I'm missing, it feels like it would be useful to set a define for the RISC-V case as well (and agree this with the GNU folks).

LGTM, thanks.

In D126079#3528290, @jyknight wrote:

In D126079#3528275, @tianshilei1992 wrote:

I did have one patch locally that only keeps the function shouldExpandAtomicInstInIR(Instruction *) and in the body it has something like if (auto *SI = dyn_cast<StoreInst>(I)). In terms of readability, IMO it is same as overload. However, of course it can reduce number of APIs. I can use that method, but it looks like the controversy here is readability.

Without seeing the result of that refactoring I can't say for sure, but I can imagine that could be an improvement over the status quo.

Thanks, I think CodeGen/WebAssembly/target-features.ll could be updated to add a -mcpu=generic test?

Oh, and per recent updates to the LLVM Developer policy I think it would be worth updating the Clang ReleaseNotes.rst to mention this change.

Based on the discussion we had, I think this makes sense. It's a bit repetitive, but could you please add a test to clang/test/Driver that checks the list of enabled features for generic (and for completeness, probably bleeding-edge as well). Thanks.

At the last sync-up call, @reames I believe said he had no further blocking comments (thanks for the review on this Philip). I think we're good to land this Lewis if you're happy with the current code.

Thanks for the patch - can you add test coverage for this please?

Add a isWebAssemblyTable helper and refactor to use it.

In D125713#3517844, @asb wrote:

In D125713#3517006, @Paul-C-Anagnostopoulos wrote:

The 'true' and 'false' literals are available.

https://llvm.org/docs/TableGen/ProgRef.html#simple-values

@Paul-C-Anagnostopoulos indeed, but they're not usable in KeyCol/ValueCols without some further changes to TableGen Element type mismatch for list: element type 'int' not convertible to 'string

In D125713#3517006, @Paul-C-Anagnostopoulos wrote:

The 'true' and 'false' literals are available.

https://llvm.org/docs/TableGen/ProgRef.html#simple-values

In D125514#3510886, @aheejin wrote:

The script doesn't generate CHECK lines next to each BB but rather generates the whole function together, making it harder to see changes in each BB for me. Also there are comments for CHECK lines, which will be lost if we use the script. So I'm not sure using it will improve readability in this case.

Added semantic restrictions for C++ specific constructs.

Rebase and fix missing CHECK lines for table copy.

This change looks fine to me, but have you considered just moving this test over to using update_mir_test_checks.py? It's ever so slightly more verbose, but in my experience reviewing changes that change test output are so much easier as you can just see the git diff of old vs new.

In D123484#3509766, @aheejin wrote:

Thanks! I have one more question but don't want to hold this back with nitpicky things. (The review turnaround time due to time zone doesn't help either)

The RISC-V changes for this look good to me.

In D124728#3507010, @efriedma wrote:

One thing that's not immediately clear to me is what the the appropriate handling of pointers with a non-default address space should be? I think specifically, atomicrmw of non-integral pointer types may have to be disallowed?

I don't see a particular problem here? I mean, an atomic xchg is perfectly sensible for non-integral pointers; semantically, it doesn't even require examining the pointer bits. I guess it's possible some targets can't implement an atomic pointer xchg, but it's not like we're forcing frontends to use it...

Thanks for working to improve our test coverage! In this particular case, I think it would be better handled by adding an RV64 RUN line to rv32zfhmin-invalid.s (given the contents of the file is identical).

Update based on review comments.

LGTM. Thanks Craig!

One thing that's not immediately clear to me is what the the appropriate handling of pointers with a non-default address space should be? I think specifically, atomicrmw of non-integral pointer types may have to be disallowed?

LGTM, thanks.

In D125354#3505989, @benshi001 wrote:

Would it be better to change the common CHECK to RV32 in rv32 tests, and RV64 in rv64 tests ?

LGTM, thanks for the cleanup. I though update_llc_test_checks struggled with these combinations but it seems not (or at least, not any more).

Nice improvement - thanks. Might be worth adding an explicit test case for where an lui+addiw is generated but the addiw can't be converted to an addi.

This refactoring looks like a good move to me.

LGTM - thanks for working through to remove these redundant comments (and fixing the missing fld/fsd/flw/fsw alias tests).

In D91719#3471226, @edward-jones wrote:

The documentation changes to the riscv-toolchain-conventions repository have been merged now, (https://github.com/riscv-non-isa/riscv-toolchain-conventions/pull/10). In addition, there's a pending patch to implement this in GCC (https://gcc.gnu.org/pipermail/gcc-patches/2022-March/592467.html). I think this means there's nothing blocking this except a rebase.

Address review comments:

• Move to using update_llc_test_checks.py for ref-null.ll (and therefore fix missing CHECK lines)
• Add tighter error checking for ref.is_null in the wasm type checker

Commandeered, completed implementation, and updated summary to explain current implementation approach.

In D123510#3465123, @rjmccall wrote:

Sounds like you need an RFC thread on the forums describing the underlying WebAssembly feature and your proposed language design for exposing it in C.

Rebase, and fix+test missed error case (taking the address of table).

Rebase.

Re-add full context.

Update to address review comments.

Update to latest version and update summary. As I say there, it would be really helpful to get some feedback at this point.

@craigblackmore: could you please take a look at compiling complex-2.c from the GCC torture suite with this patch. I'm getting incorrect output for RV32IMAFDC at Oz:

./output_rv32imafdc_ilp32_Oz/complex-2.s:16:7: error: invalid operand for instruction
        addi    fa0, ft0, 0
                ^
./output_rv32imafdc_ilp32_Oz/complex-2.s:18:7: error: invalid operand for instruction
        addi    fa1, ft1, 0
                ^

Addresses some potential null pointer dereferences.

@MaskRay: Are you happy all your comments are addressed?

@hughperkins: You make good points. It's not immediately obvious what kind of splitting would be better, and at what point the added complexity of trying to have piggy-back zfinx on current F/D patterns hampers readability and maintainability rather than enhance it. If you have time to play around with some options, I'd be really interested in hearing what you come up with.

In D123496#3445710, @Zeavee wrote:

Sorry for the delay, it took me some time to run clang-format. I also added the context as asked. Finally, I added an assert to make sure STACKMAP, PATCHPOINT and STATEPOINT are only used with RV64.

In D122918#3448001, @hughperkins wrote:

As a general comment, after thinking about this whilst brushing my teeth, I kind of feel that it might be clearer and simpler for everyone, both F people and Zfinx people, if we decoupled F and Zfinx into two separate files. This way F people don't have to figure out what is Zfinx. They just keep their clean separate F file. and Zfinx people dont have to think about how our bits mangle with F bits. Yes involves a bit of duplication, but if it's clearer/cleaner for everyone, maybe it might be a good approach?

After all, Zfinx is not an extension of F. It's a separate, mutually incompatible extension. And it's a fairly niche extension, I feel.

LGTM, thanks.

Thanks for the patch! One quick suggestion from a rapid scan is that it would be good to have test coverage for illegal extension combinations (including for .option arch removing an extension that is a dependency of another enaabled extension).

Thanks for the contribution! I'm not very familiar with the stackmap/patchpoint/statepoint infrastructure so it may take a bit of time to review this (if anyone else on the CC list has looked at it before, please do dive in!).

This LGTM - thanks!