User Details
- User Since
- Jan 4 2017, 12:12 PM (281 w, 2 d)
Wed, May 25
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?
Fri, May 20
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.
Thu, May 19
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.
Wed, May 18
Thu, May 12
There's a lot of really quite wonky whitespace reformatting in this diff
Mon, May 9
Thu, May 5
What's the justification for 0 rather than undef? That the high bits need to be guaranteed equal even for undef input?
Mon, May 2
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?
Fri, Apr 29
Apr 22 2022
Use update_mir_test_checks.py, don’t hand-write specific check lines
Apr 21 2022
What are the units?..
- The commit message for the reverting commit should explain why patch is being reverted.
Apr 19 2022
- 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?
Apr 18 2022
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.
Apr 11 2022
Apr 9 2022
The test case file names are uninformative, and having both reduce-2/3 and reduce2/3 doesn't seem like a good idea
Apr 7 2022
And can we please have regression tests for these cases?
The commit message says JumpTableIndex but isJTI is already checked?
Apr 5 2022
This will be a lot of fun downstream...
Apr 2 2022
Apr 1 2022
Mar 28 2022
Comments on the RISC-V test to keep it consistent with all the others
Mar 23 2022
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.
Mar 21 2022
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.
Mar 18 2022
There's a whole set of these you could do; why this one in particular? And what about it makes it RV32-specific?
Mar 17 2022
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?
Mar 16 2022
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.
Mar 15 2022
- Renamed test case
- Delete TODO as fixed
Mar 14 2022
Mar 4 2022
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
Mar 2 2022
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?
Mar 1 2022
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.
Feb 27 2022
Feb 26 2022
Feb 24 2022
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.
Feb 18 2022
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?
Feb 17 2022
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
Feb 16 2022
Feb 12 2022
Reverted due to https://github.com/llvm/llvm-project/issues/53662
I objected and still believe this patch is fundamentally wrong. The problem needs solving elsewhere, not like this. Please revert.