e70d4994ea9eb44ba5a86c5073ac83cf500aabdd should have the suggestions incorporated.

Interesting, are the logs from the runs available? I have run the test ~10000 times locally and its been stable. Perhaps the logs can show what is going on.

Address feedback from @MaskRay

I'll address the clean ups in a follow up.

LGTM; do you need someone to commit this on your behalf?

I was able to play around with this further yesterday evening. You are correct - the issue is the load preventing the watcher thread from spinning up. I was able to reproduce this issue and resolve it by adding in a synchronization point (boolean + mutex + condition variable) before returning the directory watcher to ensure that the RDC was setup. I looked through all the previous failure cases as well as the one that I was finally able to reproduce - it is always the initial notification that we missed (because the thread took too long to come up). In the process I did do a few minor alterations as well. I would like to get additional testing over the weekend on the bots so people aren't impacted if there turns out to be some other subtle threading issue. However, running this in a tight loop locally seemed to be pretty stable (switching the builds to a SSD locally did help uncover the flakiness) so I am confident that this should be safe. I'm happy to address any additional comments in post-commit review.

Some platforms (e.g. WoA) do require thumb-only compilation. Can we conditionalize this on whether we are building for ARMv6 or not?

Given the thread on the mailing list, I think that this is fine as long as there is an accompanying test to validate the bitcode deserialization continues to work. Id wait a few days more to see if @dexonsmith has an opinion.

2 remaining failures KI:

• debug_line encoding is not relaxable

3 failures left now. KI:

• 6B relocations are not implemented
• debug_line references aren't correctly emitted

Down to 6 failures to explore.

7 failures to look into; need to still replace the use of RelaxAll which would also improve the test pass rate.

Closer to passing tests (9 more failures to understand). It matches gas' behaviour better, is more uniformly behaved.

I’d say wait a day or so to make sure that @ldionne doesn’t have any concerns, but this seems like it should be safe. Rebasing a pointer seems like a useful enough thing and this don’t impact the public interfaces.

Shouldn't this be covered by option-relax.s already though? Is that test wrong?

I don't understand the diff rendering on differential. The change is literally the 3 lines being added to RISCVTargetELFStreamer::emitDirectiveOptionRelax. The raw diff does not show any changes to the other lines around it.

Thanks for adding that comment - I think that the cleanup routine may be confusing otherwise.

Hmm, actually looking at gcc, it appears that this is actually matching semantics with gcc:
https://github.com/riscv/riscv-gcc/blob/03cb20e5433cd8e65af6a1a6baaf3fe4c72785f6/gcc/config/riscv/riscv.c#L4590-L4593

Ping; it seems like there aren't any objections, can I get a final sign off on the change?

If the user is passing along -mno-relax encoding that in the object file isn't expensive, and it doesn't harm anything. In fact, the user _could_ at a later point pass in -mrelax to the assembler to override the beahviour (with a warning). But what @raj.khem states about the non-IAS case should hold - -mno-relax should get passed along to the assembler along with the hint in the object file.

While I can appreciate the removal of deprecated functionality, I think that some sort of mention of this on llvm-dev at least 2 weeks prior to the removal is warranted. It impacts any downstream projects which may still be relying on a feature.

@jrtc27 ping

@jrtc27 - I'm open to improving the diagnostics, but this plumbs the functionality.

I don't disagree about the ease of updating that code (and, in fact, I've created PRs #241, #242, #243 on the project). However, the issue here is more than just the single project.

Sure, but even if they were to deprecate them, that would be years before code would really be even nudged towards removal. In the mean time, this prevents the use of the IAS for building software, which is rather unfortunate. I think that supporting the aliases and adding a deprecation warning on use seems to be better. That way, we can actually both encourage people to update the code as well as remain compatible.

Hmm, perhaps we should be special casing these and adding diagnostics then. The thing is, that binutils still carries the aliases, and generally, LLVM has strived for compatibility with binutils so that it can remain a drop-in replacement.

While I think that this is fine in general, I'm not sure why the shift operator needs to be replaced. Could you shed some light on that?

This is a pretty straightforward cleanup now, which adds additional functionality by deferring work to CMake. There are a couple of minor points about inconsistent quoting but this seems good otherwise.

Thanks!

Thanks @phosek; I've not accepted as libc++abi or libc++, as I want to ensure that @ldionne doesn't rely on this functionality.

I don't think that LC_FUNCTION_STARTS would create an actual section.

Im afraid I dont understand the purpose of this change. libunwind isn't meant to be merged into libc++/libc++abi. The reasoning for libc++ to support merging libc++abi doesn't really apply to libunwind IMO. Can you please elaborate what exactly the reasoning for this is?

Thanks for the updates!

Ah right, the !RISCV -> RISCV case. This makes sense to commit to fix the immediate issue.

This is fine for now, but in the future, for such changes, please split this up into a series of patches. The clean ups for the named constants and clean up for floating point handling could have been separate changes which would have reduced the overall size of the changes and focused the patch specifically to adding support for rv32.

Thanks!