LLVM, OpenTitan, messing with hardware, learning stuff... life is fun.
- User Since
- Nov 2 2018, 7:48 AM (255 w, 2 d)
Jun 6 2023
May 25 2023
Apr 6 2023
Thanks for all of the improvements so far.
Mar 24 2023
Adding a test showing that using .option arch in inline assembly doesn't affect ELF attributes nor set the EF_RISCV_RVC ELF flag would be nice. The latter can also be tested for assembly files.
Overall LGTM. Only some minor points left to address.
Mar 14 2023
Mar 6 2023
Mar 5 2023
You now accept .option arch, rv32ic (without the quotes) but that's not covered in the valid tests.
Also, you're normalizing that to the quoted version .option arch, "rv32i2p0_c2p0" and accepting that but, at first glance, I'm not seeing that as an accepted case in the binutils tests.
Update with the re-landed patch.
Mar 2 2023
From Kito's comment in D144174: https://github.com/gcc-mirror/gcc/commit/3cd08f7168c196d7a481b9ed9f4289fd1f14eea8
Feb 28 2023
Yes, this can land independently of the other work. Since D144839 was abandoned I'll update and land this.
I'll also work on the unit test and llvm-symbolizer improvement, I've just been busy with childcare and other work.
@StephenFan Can you please rebase this?
Feb 19 2023
I suppose we could also have added S-type instructions tests, for thoroughness.
Feb 16 2023
The reasoning and approach makes sense to me. I haven't (yet) checked the implementation and test coverage in detail, so this is a weak approval only.
Feb 15 2023
Feb 10 2023
Feb 9 2023
Does this mean that Android, OpenBSD, and Cygwin have emulated TLS support for all supported architectures other than RISC-V?
Or are you essentially asserting, by making this contribution, that we should add additional opt-outs for other architectures?
Feb 8 2023
Feb 6 2023
LGTM. It might have been needed in an older version of D62592.
Feb 5 2023
Jan 30 2023
Jan 29 2023
Overall seems a reasonable implementation, despite the limitations pointed out in the patch description.
Improve LLVM directory search.
Jan 26 2023
LGTM, with some in-line caveats.
Possibly we want to add this change to the release notes?
Jan 25 2023
The lsan tests fail when SANITIZER_CAN_USE_ALLOCATOR64 is defined as 1, both on rv64 sv39 and sv48 hosts.
It might be a good idea to see the compile-time impact of this with https://llvm-compile-time-tracker.com?
With the current (unpatched) LLVM tree on an RV64 SV39 host I get this ASAN test failure (for check-asan):
Jan 24 2023
Makes sense to me and the results I'm seeing are reasonable. (this is also a weak approval)
I'm probably not the best person to review this but, for what it's worth, it LGTM.
Do we actually want to include the dso_local in the full signature?
Jan 23 2023
LGTM but others should also chime in.
Jan 21 2023
Jan 20 2023
Jan 19 2023
Jan 4 2023
Nov 29 2022
Nov 22 2022
Nov 21 2022
I was assuming this would never increase code size but that didn't seem to be the case. Since it wasn't clear to me why that would be, I reduced a problematic case and the conclusion is... it's because it's incomplete. The new register allocation sometimes ends up being unfavorable for the compressible instructions that are not yet handled (obvious in hindsight). I presume that once this code is complete that will either go away or be much more unlikely. Still, even as it is, the typical result is to improve code size so it's a nice improvement!
Nov 9 2022
I see no reason not to accept this. I've checked that the tests pass.
Nov 7 2022
As far as I can tell from looking at the relevant psABI discussions and GNU patches, this overall approach is OK.
Oct 31 2022
I looked at this more carefully. I think the CHECKs for the .section were already not being updated automatically (not sure if that was the case when they were originally added), so you didn't change that (when I originally skimmed the patch I thought that was new). I'm not sure the two UTC_ARGS are actually needed.
Oct 27 2022
Oct 26 2022
Sorry, could you please explain why this required a manually written check? It's not obvious to me.
Is the tagging actually incompatible with the code model or only with the address materialization instructions that we use as part of the implementation of the code model? Does the tagging actually change the location of globals? I was assuming the location was the same, and the tag was just changing how we encoded that location in a numerical value. My understanding of the code model specifications is that they only care about the actual address ranges. Although the RISC-V psABI gives examples of instruction sequences my interpretation of that is that it's non-normative. So if the location doesn't actually change in principle you could materialize those with a different instruction sequence and still claim you comply with the code model, is that not the case? That only impacts the comments in the patch, I'm not opposed to the approach of using the GOT.
Sep 20 2022
Regarding the overkill of "RISCVInstrInfoZawrs.td", how about having a "RISCVInstrInfoExtra.td" (or "RISCVInstrInfoExt.td") as a grab bag for everything that doesn't merit its own .td file?
Aug 25 2022
Aug 24 2022
The tests don't seem to have been properly updated with update_llc_test_checks.py. llvm/test/CodeGen/RISCV/branch-relaxation.ll contains RV64 RUN lines but the corresponding CHECK lines are missing in some functions.
Looks correct to me now. Let's wait a little bit to see if there's more feedback from others.