Mostly LLVM work.
- User Since
- Nov 2 2018, 7:48 AM (136 w, 6 d)
The last time I looked at this I had some concerns but I didn't have time to fully investigate them.
To help get this going again, could you please clarify:
Tue, Jun 15
Wed, Jun 9
Mon, Jun 7
Are you planning on extending this to use bclriw on RV64? (Would that clash with the use of tablegen? Personally, I tend to find C++ custom lowering of longer stuff easier to read and more cohesive, not sure how others feel)
Mon, May 31
Thu, May 27
I'm going to try to review this soon.
Wed, May 26
I'm not very happy that we now have to consistently check two variables (error-prone, etc.), but I suppose that's OK if we are going to remove -riscv-no-aliases soon.
Other than that, the patch LGTM.
May 13 2021
We need to optimize integer materialization, both in general and to take advantage of bitmanip. I suppose that the baseline test would be less bad if we had done that, but that this optimization would still be worth it?
I've benchmarked the impact of this patch with CoreMark and Embench and it looked good. In summary, there were no or minimal performance differences but there were various small improvements to code size, which are probably similar to your test case.
I was going to add other checks like running the llvm test suite but the patch no longer applies. Can you refresh it?
May 11 2021
I'm not the best person to review test-suite patches, but I also don't see how this could be wrong, so I'm approving it.
There are also Clang test failures that need to be addressed before committing.
May 10 2021
Apr 29 2021
Address review feedback.
Apr 28 2021
LGTM. Thank you!
Rebase and fix the unwinding issue reported by @jade.
Apr 27 2021
The SelectionDAG changes LGTM.
Apr 15 2021
Thanks, good catch. LGTM.
Apr 13 2021
Apr 7 2021
Could you please clean up the test a bit? It contains references to attribute groups that don't actually exist (#0, #1). One attribute you do want to add is nounwind, to avoid the clutter caused by the CFI directives. In manually written tests we also generally don't include some of those dso_local, local_unnamed_addr, etc. In general, it would be nice to make it look less like Clang's output and more like something that can be easily read and reasoned about.
Optimize address mappings.
Apr 6 2021
Apr 5 2021
Once update_mir_test_checks.py is tweaked we can start using it for this test, but let's merge it as-is for now. Thanks!
Apr 4 2021
Just a heads-up about a possible pitfall of this patch, though I'm not sure it actually affects SystemZ.
Fixing alignment before truly abandoning, in case it's useful for D97514.
BTW, it shouldn't affect correctness but the original CreateStackTemporary would use (as part of that method's implementation) getPrefTypeAlign instead of getEVTAlign. In principle that would allow better alignment choices, although at the moment it probably would never make any difference.
Apr 1 2021
Mar 31 2021
Mar 30 2021
Address review feedback (thanks @MaskRay!).
Tested with the sanitizer-x86_64-linux-android and sanitizer-windows buildbots, so this time there shouldn't be buildbot failures when committing.
- Add VarArg handling.
- Adjust tests.
Mar 23 2021
@sequencer What's the plan for this patch?
There are certainly fewer changes to existing tests, compared with
Mar 22 2021
- Precommitted the original test, to better show the difference caused by the patch.
- Updated the existing tests. They seem to more broadly show the issue I noted about the excessive alignment/overly conservative stack size.
Mar 20 2021
Superseded by D77384 and D97490.
I can confirm that the mips* XFAIL now passes (since D77384 landed on Feb 26). If wasm contributors are still interested in reviewing this patch I can update it to include only the immediate.ll test update.
Mar 18 2021
I just looked at this again and I don't have the full context in my mind right now but won't the test just exercise the BareMetal toolchain and not your changes?
Mar 17 2021
Address review feedback.
Before relanding, I need to check/fix the SizeClassAllocator64PopulateFreeListOOM test for Android x86, as the respective buildbot failed in the first landing attempt.
Fix silly mistake in the last update.
Mar 16 2021
This still doesn't report that the multilib configuration came from GCC when it succeeds, does it? I suppose that's not a deal-breaker, but it would be nice to have. Would it be difficult to implement?
Mar 15 2021
This broke debug info for RISC-V. Test case with broken symbolization: