User Details
- User Since
- Aug 6 2013, 5:31 AM (459 w, 3 d)
Today
Yesterday
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);
Wed, May 25
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.
Sat, May 21
Thu, May 19
Tue, May 17
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.
Mon, May 16
Added semantic restrictions for C++ specific constructs.
Rebase and fix missing CHECK lines for table copy.
Fri, May 13
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.
Thu, May 12
The RISC-V changes for this look good to me.
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).
Wed, May 11
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.
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).
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
Tue, May 10
Commandeered, completed implementation, and updated summary to explain current implementation approach.
Apr 26 2022
Apr 25 2022
Rebase, and fix+test missed error case (taking the address of table).
Apr 22 2022
Rebase.
Re-add full context.
Update to address review comments.
Apr 21 2022
Update to latest version and update summary. As I say there, it would be really helpful to get some feedback at this point.
Apr 20 2022
Apr 14 2022
@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.
Apr 13 2022
LGTM, thanks.
Apr 11 2022
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!).
Apr 7 2022
This LGTM - thanks!