In D146847#4220721, @schittir wrote:

In D146847#4220697, @jrtc27 wrote:

None of the other fields are initialised, so blindly initialising it alone to 0 here seems highly suspect. What's the actual case in which it's used whilst uninitialised?

Coverity complains about SR.PolicyBitMask being uninitialized when calling SemaRecords->push_back(SR)

The field should just be deleted. D126742 added it without any uses, and I don't know what the original intent was. Maybe it was used before the bitfields were added and it stuck around rather than being GC'ed. I don't know why Coverity would care about this though when it's never read from, there's nothing wrong with that, unless the warning is in fact that the field is entirely unreferenced?

None of the other fields are initialised, so blindly initialising it alone to 0 here seems highly suspect. What's the actual case in which it's used whilst uninitialised?

Are there any cases like with all the *W optimisations where it would be better to sign-extend, or is i32 special there due to the *W ops and for i8 the only cases where it's beneficial will already have become sextloads?

Is there a reason we can't instead add the right live-outs?

Tests?

In D123515#4189301, @hiraditya wrote:

This patch is needed for zbb support as per: https://lore.kernel.org/all/20230117122447.y6tdsmsxqdwf76ri@orel/

What happened to your new test?

Anyway, this definitely still merits a review from @MaskRay given his commit to tackle the problem differently

Anyway this seems to contradict the approach chosen by D127826

In D134129#4179673, @MaskRay wrote:

At least with that this diff shows, the saving is small. I'd hope that we don't toggle -msave-restore for -Os/-Oz. Users can specify -msave-restore themselves.

In D145539#4176834, @luojia wrote:

In D145539#4176831, @jrtc27 wrote:

In D145539#4176824, @luojia wrote:

In D145539#4176791, @jrtc27 wrote:

I'm guessing this is so you can llvm-objcopy an ELF into a PE/COFF?.. Because without relocations being defined you can't do much else of use...

Yes llvm-objcopy can do this and this is the current approach on EDK2 RISC-V. But for languages like Rust it will be more convenient if the compiler backend writes it out directly from build target configuration

And how are you going to do that without a full PE/COFF specification for RISC-V?

Thanks for mentioning! If I am correct, PE/COFF specification has definitions of machine architecture in headers. In this part PE/COFF included RISC-V architecture types. It does not provide relocation types on RISC-V, so for now PE/COFF specification allows us to emit RISC-V COFF binaries without any relocations. This could be a use case for RISC-V UEFI applications where it's mainly statically linked without any relocations. If there are more demands on RISC-V PE/COFF which need relocations, the current PE/COFF specification will not support them and LLVM (after this patch) would raise fatal error instead.

I have limited information about RISC-V PE/COFF support as well; I don't know if other reviewers have different opinion about this part.

In D145539#4176824, @luojia wrote:

In D145539#4176791, @jrtc27 wrote:

I'm guessing this is so you can llvm-objcopy an ELF into a PE/COFF?.. Because without relocations being defined you can't do much else of use...

Yes llvm-objcopy can do this and this is the current approach on EDK2 RISC-V. But for languages like Rust it will be more convenient if the compiler backend writes it out directly from build target configuration

I'm guessing this is so you can llvm-objcopy an ELF into a PE/COFF?.. Because without relocations being defined you can't do much else of use...

This isn't NFC, the output changes

In D140478#4168115, @dblaikie wrote:

In D140478#4165992, @jrtc27 wrote:

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

In D140478#4165992, @jrtc27 wrote:

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

It's a per-function feature that has implications for other functions in the same section (deleting code in one function at link time will shunt around the code in the rest of the section).

What does it mean for it to be per-function? Can it vary at all between functions in the same module? (if not, why is it per-function?)

A cursory glance reveals numerous cases of ( i1, and cases of undefined variables (e.g. llvm/test/CodeGen/PowerPC/subreg-postra.ll).

In D140478#4165988, @dblaikie wrote:

In D140478#4164792, @RamNalamothu wrote:

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Then won't this make the wrong decisions if the value varies between functions/subtargets?

Also, at the moment, the -mrelax/-mno-relax affects the entire compilation unit for the RISC-V.

Then wouldn't these be target features, rather than subtarget features? My understanding is that subtarget features generally can vary by function, whereas target features cannot.

Even if the command line flags don't support it, we could probably make a simple IR example with different relaxation per function? & so we probably should be able to handle that case.

In D143115#4149693, @asb wrote:

@kito-cheng @jrtc27: Can I just confirm the ABI development process? We obviously went through a whole ratification exercise before, but how does it now work for these kind of additions? I'm mainly concerned about merging support for something that's added to the psABI doc, but then having to change it later if it's viewed as not "final". I guess this shouldn't happen, as a consequence of the PR merging policy but it's still a bit unclear how it intersects with any future ratification process that might be requested by RISC-V International.

Though FWIW, I think the path of getting broad consensus on the proposal and then considering it 'done' is just fine - I'm not sure extra layers of sign-off would be helpful for incremental improvements like this. And in case I missed a hidden layer of bureaucracy, I _think_ the x86_64 psABI is evolved in a similar way.

Normally we use the less cumbersome “MC layer” rather than “assembler and dis-assembler” (which also shouldn’t be hyphenated)

In D143673#4142568, @craig.topper wrote:

@MaskRay here's some data I collected on SPEC2006 .text size and dynamic instruction counts with and without GP relaxation using the GNU linker. I also measured using GP as a callee-saved register. https://docs.google.com/spreadsheets/d/14V7cPbyc80AcGHzsMaw9hYb232dzRbGCmTApnxj-SpU/edit?usp=sharing

Didn't notice before, but why emutls_generic.ll rather than emutls.ll? The suffix doesn't add any value as far as I can see. It's as generic as any other RISC-V CodeGen test.

Whilst I would encourage wherever this got copied from be cleaned up in a similar manner, I don't think:

1. this patch for one backend should be blocked on cleaning up a test in another backend
2. a lower-quality test for another backend should be justification for committing a lower-quality test to this backend

https://github.com/riscv/riscv-isa-manual/releases/tag/archive has an assortment of past specs, including the 2.2 you're after

In D143708#4133064, @MaskRay wrote:

In D143708#4132222, @asb wrote:

The consensus from the RISC-V LLVM sync-up call was that it makes sense to go ahead with this on the basis their are users, the infrastructure in LLVM already exists to use it etc, and it's a tiny patch to enable etc.

In which case, assuming no-one who wasn't able to make it to the call wanted to raise strong objections, we can hopefully focus on code review of this patch and move it forwards.

I have a mild objection but I am fine if my opinion is overthrown. I think OpenBSD developers should take on the work to migrate to ELF TLS, not using this definitely.

-femulated-tls can be used for the current *-linux-android* and *-openbsd* target triples, and OpenBSD RISC-V, but not others. The test should be changed to use openbsd, not linux-gnu.

In D143982#4125543, @jrtc27 wrote:

You don’t need to mention FCVTMOD given it does not apply to C/C++.

In D143345#4130588, @luismarques wrote:

In D143345#4130585, @craig.topper wrote:

It'd be really cool if we could use the relocation to print %pcrel_lo(sym) instead of 0. But I don't know how much work that would be.

I can work on that if there is agreement that we should go in that direction. Makes sense to me, but I wonder if some people will push back against that, regardless of implementation concerns.

In D143248#4128698, @varunkumare99 wrote:

In D143248#4122933, @asb wrote:

Hi Varun - thanks for filing bug #60698 to help track this issue, and for noticing that there should now be some upstream support for cfi_remember_state and cfi_restore_state.

The first question that comes to my mind is what degree of testing you've done with this?

Hi Alex,
the patch updates over 500 testcases for RISCV(as it adds cfi info the epilogue), which results in a patch of over 30M. Due to which I'm not able to upload.

You don’t need to mention FCVTMOD given it does not apply to C/C++.

Personally happy with the concept then, seems consistent and overall helpful, just some nits

In D143953#4124636, @reames wrote:

@jrtc27 Not sure if this changes your take, but I realized the variant being introduced is maybe much less interesting than I'd first thought. We generally make no effort to make sure an extension was defined in the spec version corresponding to our base revision. Given that, we have a bunch of cases where we allow I2.0 + some random extension. Given that, this one stops looking all that interesting. It doesn't actually set much precedent - because we already did that, a long time ago.

If you agree with that framing, I'll rework the description.

I'm fine with this approach but I think they should be filtered out of the ELF attributes, maybe also preprocessor macros? That is, treating them as a redundant no-op in the driver rather than like a true extension.

In D143619#4117399, @vit9696 wrote:

@paulkirth, I agree that shipping a working compiler is more important than parity with the other compilers, but in this particular case we simply face a simple to fix bug caused by component owner tyranny :) We should simply fix this bug and move forward.