Technically it's may not will so you can just hard-wire it to 1...

For reference, this is the exact text in the psABI spec:

Doesn't this run the risk of temporarily misaligning the stack (e.g. for Val == 2064)? The psABI says using such a non-standard ABI within a function is fine and that kernels should realign stacks before calling signal handlers. Both FreeBSD and Linux correctly do this for signal handlers so as to not make assumptions about whether userspace is following the standard ABI, but both assume that the kernel itself is following the standard ABI, and so do not realign the stack in their exception entry points, causing the stack to be misaligned if an interrupt is taken in between. Whether this is a bug or not in the kernels is debatable, but with a more careful splitting of Val it should be possible to avoid anyway?

Is this meant to be "it works" or "it works without trapping for emulation"? Pretty much every EEI out there has misaligned accesses guaranteed to work, just not quickly, and in those cases you'd still want to avoid them as the inlined byte-wise code is far faster.

Also, the tests where you have codegen changes rather than preserving a soft-float ABI should probably be put up for review separately by adding an explicit hard single-float ABI, as those seem worthwhile for reducing noise

It's currently this way in order to be compatible with GCC. Changing this requires consensus from both toolchains to ensure compatibility is preserved. See https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/13 for some discussion on this.

In D124212#3509972, @eugenis wrote:

In D124212#3509846, @glaubitz wrote:

In D124212#3509781, @jrtc27 wrote:

There's a lot of really quite wonky whitespace reformatting in this diff

FWIW, I have already a draft for fixing this. Will create a revision tomorrow.

EDIT: I mean the build failure on sparc64 ;).

Thanks. Sorry for the breakage. It appears that sparc64 is the only 64-bit target without newfstatat syscall.

There's a lot of really quite wonky whitespace reformatting in this diff

What's the justification for 0 rather than undef? That the high bits need to be guaranteed equal even for undef input?

What happens when we want to use LLD-specific options or are checking LLD-specific messages? Those tests aren't going to pass. Do we then need to annotate LLD tests with REQUIRES: lld-is-actually-lld? Is there any precedent for doing this on other architectures? This doesn't feel right to me.

Why can't people just symlink GNU ld to ld.lld?

Use update_mir_test_checks.py, don’t hand-write specific check lines

What are the units?..

• The commit message for the reverting commit should explain why patch is being reverted.

• Although I haven't run into that problem, I think it's still possible for an ADDI to become out of range of the AUIPC it refers to, if other passes move them too far apart. For branches we have the branch relaxation pass to solve essentially the same problem. Are we going to need a similar pass for AUIPC/ADDI? Or can we prevent this from happening?

Might be clearer as let isCommutable = 1/true in on each def? There aren't that many of them, so that keeps it self-documenting and avoids long argument lists.

a could be preempted, so no, it has to go via the PLT for that specific example.

The test case file names are uninformative, and having both reduce-2/3 and reduce2/3 doesn't seem like a good idea

And can we please have regression tests for these cases?

The commit message says JumpTableIndex but isJTI is already checked?

This will be a lot of fun downstream...

Comments on the RISC-V test to keep it consistent with all the others

As a developer who often deals with crashes locally this is more annoying; currently I can just point tools at the shell script and C file in /tmp and let them go to work reducing, but now I have to also extract the files

Address spaces can also be used for the exact same memory but a different representation of the pointers, which can come with semantic differences; we use them in CHERI LLVM to differentiate between traditional integer addresses and CHERI capabilities, with the latter having associated bounds and permissions to provide spatial safety.

Are you sure this is a code size win with RVC? Also much less likely to be macro-op fused; conditional branch followed by a move is one of the things you can easily fuse, and some cores already implement predicated execution for short forward conditional branches.

There's a whole set of these you could do; why this one in particular? And what about it makes it RV32-specific?

In D121654#3391091, @kamaub wrote:

In D121654#3390916, @jrtc27 wrote:

Does:

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f52965dab759..299c68da7c2f 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -3769,6 +3769,15 @@ SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
   }
 }
 
+template SDValue RISCVTargetLowering::getAddr<GlobalAddressSDNode>(
+    GlobalAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<BlockAddressSDNode>(
+    BlockAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<ConstantPoolSDNode>(
+    ConstantPoolSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<JumpTableSDNode>(
+    JumpTableSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
   SDLoc DL(Op);

fix the error for you?

I'd be happy to review and approve a patch with this change but I think the fastest resolution to getting the bot back to green would be to revert and then recommit with this change added. The bot has been broken for over 24 hours so we would like to get it back online as soon as possible, thank you for your quick response. :)

Does:

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f52965dab759..299c68da7c2f 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -3769,6 +3769,15 @@ SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
   }
 }

My guess as to what the problem is is that this has perturbed inlining heuristics such that the ConstantPoolSDNode template instantiation no longer exists in the output object file, as we don't explicitly instantiate any of them, only implicitly via their uses?

In D121654#3390865, @kamaub wrote:

I have confirmed that this patch cause a miscompile on our sanitizer-ppc64be-linux #4255 bot, please investigate and fix this as soon as possible, thank you.

This then duplicates the AUIPC though and scales as 2*N rather than N+2. If you want to do this properly you really need to have a peephole pass that runs after the pseudo has been expanded.

• Renamed test case
• Delete TODO as fixed

With one exception, every RISC-V Clang CodeGen test (and with 9 exceptions, every RISC-V LLVM CodeGen test) is kebab-case not snake_case

Never mind, I see you added a test for that case

Does a double with r for RV32 work with that fix? That's supposed to give the low half of the register. You might need to also deal with the register pair class?

Why do you need to derive from any of them. Just modify them. It's so much more painful to do it the way you're suggesting. I say this as a downstream that makes significant changes to instruction selection and adds totally new ABIs.

Note that git grep ' : public TargetPassConfig' llvm/lib/Target turns up 21 results, exactly one for each backend, and every single one lives in the backend's TargetMachine.cpp. So moving it to a header, or adding a second one, would go against the convention of every single backend in tree.

Inheriting from the pass doesn't make sense, just change the implementations. The TargetPassConfig has access to the TM for a reason. As someone with significant downstream changes I am sympathetic to changing upstream code to be more accommodating, but this change just seems motivated by an arbitrary downstream decision that goes against the intent of LLVM's structure just to avoid the occasional merge conflict (RISCVPassConfig hardly changes much).

We already rejected this in the past. They’re unnecessary if you don’t bogusly drop flags when assembling.

In D120518#3344165, @arsenm wrote:

In D120518#3344148, @jrtc27 wrote:

This isn't const *correctness*, this is adding a new restriction. The existing code is totally correct, but this potentially makes downstream code no longer correct.

Maintaining state in TTI is definitely bad, so this is a good restriction

This isn't const *correctness*, this is adding a new restriction. The existing code is totally correct, but this potentially makes downstream code no longer correct.

In D118987#3319697, @steakhal wrote:

It seems like the clang-ve-ninja doesn't really want to accept any patches from me :D
I hope it's not personal. Let's be friends bot, please.

Link to the breakage: https://lab.llvm.org/buildbot/#/builders/91/builds/3818

I'm inviting @simoll for resolving this, and the underlying issue to prevent future breakages and reverts.

I'm unconvinced this is quite the right fix for the bug. I agree we need this code, but I disagree that this code should be needed when Zfinx isn't in use. Without Zfinx, we should never have GFPR* be legal in the first place, surely?

In D120130#3332072, @achieveartificialintelligence wrote:

In D120130#3332052, @xbolva00 wrote:

Missing test

Tests are in the rust environment.

In D93298#3332038, @xbolva00 wrote:

It appears that this is causing an assertion segfault in a rustc test over at our experimental rust + llvm@head bot:

I dont think that patch author is required to debug this issue for "experimental rust + llvm@head" - downstream.

Since I don't have a rust environment

There is no requirement, indeed. LLVM devs are not responsible to keep ""experimental rust + llvm@head" working.

In D93298#3332020, @achieveartificialintelligence wrote:

@krasimir Since I don't have a rust environment, can you help me to test if D120130 works?

In D93298#3331641, @krasimir wrote:

It appears that this is causing an assertion segfault in a rustc test over at our experimental rust + llvm@head bot:
https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/8430#167e6de5-2dd5-41c3-87d7-b6e3f3908371/262-706
The test is https://github.com/rust-lang/rust/blob/master/src/test/assembly/asm/riscv-types.rs. These two lines appear to cause it (code compiles fine when removed):

• check_reg!(a0_f32 f32 "a0" "mv"); https://github.com/rust-lang/rust/blob/f838a425e3134d036a7d9632935111a569ac7446/src/test/assembly/asm/riscv-types.rs#L178
• check_reg!(a0_f64 f64 "a0" "mv"); https://github.com/rust-lang/rust/blob/f838a425e3134d036a7d9632935111a569ac7446/src/test/assembly/asm/riscv-types.rs#L192

The assertion:

Impossible reg-to-reg copy
UNREACHABLE executed at [...]/rust/src/llvm-project/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:350

...

Revision summary seems to be totally lacking any explanation of what this actually is for. Also how is this meant to interact with architectures like MIPS where return value registers aren't argument registers (v0 and v1); is it meant to include those or not? The name suggests it does, but the old function name suggested it didn't (and was just a stub outside of X86?).

This makes sense to me but I don't know if you want someone with more authority in these parts to review it

Thanks, looks good to me

In D119934#3328037, @craig.topper wrote:

In D119934#3328030, @jrtc27 wrote:

In D119934#3328022, @craig.topper wrote:

I suppose we inherited this bug from PowerPC. @nemanjai maybe you want to fix this for PowerPC?

X86 is the same from the looks of it. The only other implementation of PostprocessISelDAG is AMDGPU which does things a bit differently and doesn't seem to have an equivalent "check for uses".

With the current post-processing on X86 I don't thin you could get a failure. None of the opcodes that are being looked for have chain outputs so they can't be the root.

In D119934#3328022, @craig.topper wrote:

I suppose we inherited this bug from PowerPC. @nemanjai maybe you want to fix this for PowerPC?

Reverted due to https://github.com/llvm/llvm-project/issues/53662

In D118216#3316773, @benshi001 wrote:

In D118216#3316752, @jrtc27 wrote:

I objected and still believe this patch is fundamentally wrong. The problem needs solving elsewhere, not like this. Please revert.

Although this solution is far from perfect, it does improve code quality. Could you please show a test case that gets wrong or worse assembly by this patch?
I suggest we can keep it and go on searching better solutions.

I objected and still believe this patch is fundamentally wrong. The problem needs solving elsewhere, not like this. Please revert.