- User Since
- Sep 26 2016, 7:58 AM (115 w, 1 d)
Tue, Dec 4
Thu, Nov 29
Might wait a little before you land this to see if @uweigand got anything more to say. But afaict this only removes the warning without affecting the result in any way, so it should not make anything worse.
Got a feeling that I'm the one who has been questioning this solution. And I guess I also kind of blocked the progress in D54962.
Anyway, I haven't really got any better alternative right now (at least not that is easy to implement).
Wed, Nov 28
Tue, Nov 27
Thu, Nov 22
I don't have any objections to the patch myself, but I don't really have the knowledge to understand if this could be bad for some important use cases either. A user can ofcourse override the threshold if needed, so I guess that makes this patch a little bit less "dangerous".
Wed, Nov 21
Tue, Nov 20
Basically I think the code looks ok. However, I'm not so familiar with this algorithm so it is hard to comment about the actual solution.
Wed, Nov 14
Nov 11 2018
LGTM now (might wanna wait for @skatkov to take a look at the last changes as well).
Nov 10 2018
Maybe the register allocator should add the implicit-def as an IMPLICIT_DEF instruction just before the MI instead of attaching a bogus impl def on the MI (if the MI uses parts of the register I guess the IMPLICIT_DEF needs a corresponding implicit use).
Nov 9 2018
Nov 8 2018
Nov 7 2018
I'm no expert on how things are divided between InstructionSimplify and InstCombine, but shouldn't the simple folds be in InstructionSimplify?
For the record, we were planning to upstream something like this in simplifyBinaryIntrinsic in InstructionSimplify.cpp:
case Intrinsic::sadd_sat: // X + 0 -> X if (match(Op1, m_Zero())) return Op0;
Nov 5 2018
If I remember correctly a problem was that depending on the order iteration at line 3011
the end result (which PHI node that others are found out to be equivalent to) could be different. Quite annoying when trying to debug something, and when turning on -print-after-all or rebuilding using -g the result is different.
Oct 30 2018
Oct 27 2018
nit: there is a spelling error in the title "Intirnsics"
Oct 26 2018
This should fix the problem discussed here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20181022/596459.html
But as shown by @test4 in the added test case (for CHECK-BE) we also get forwarding in that test now.
Oct 25 2018
The patch will avoid the assert seen in PR39417 so that is great.
(hard to tell if it still is possible to hit the assert, PR39417 was triggered during fuzzy testing using csmith to generate the input)
Oct 24 2018
Oct 19 2018
Oct 18 2018
Still no test case :-(
Oct 17 2018
Is this intrinsic really needed for the fixed point support?
Oct 15 2018
Have you considered making a .mir-test that show the effect of TwoAddressInstruction pass here. Maybe an overkill since there already are plenty of test cases that are affected by the change, but it would make it easier to understand the tranformation difference that we introduce in the TwoAddressInstructionPass.
Just some inline nit:s about whitespace.
I think this will give problems when building with clang-tools-extra, and having -Werror.
../tools/clang/tools/extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:22:11: error: enumeration value 'STK_FixedPoint' not handled in switch [-Werror,-Wswitch]
Oct 14 2018
Oct 12 2018
Oct 9 2018
If the use of a special intrinsic for saturation is rare (few targets that supports it natively), then perhaps we should put this particular patch on hold for now?
Oct 3 2018
I don't know the history behind the inferred classes, I thought they were generated because they might be needed :-)
Sep 30 2018
Sep 28 2018
Minor refactoring/renaming as suggested by Matze.
Removed the .ll test case (and updated comments in the new .mir test) as suggested in the review comments.
- Added livevars and verify-machineinstrs to the RUN line in the test case.
- Solved the fault detected by the verifier, i.e. incorrectly setting of killed on %0 in the inserted COPY instruction in test2.
Sep 27 2018
This is slightly related to https://bugs.llvm.org/show_bug.cgi?id=39085, but this patch is supposed to focus on getting IMPLICIT_DEF when all PHI inputs are undefined.
The PR is about the problem with updating "killed" that is mentioned in the "bar" part of the test case.
Sep 26 2018
The FPR64 and VR128 classes are a little bit unusual(?) as they only have one subreg, that maps to the high part of the register.
Sep 24 2018
Sep 21 2018
Addning a hand-crafted reproducer for Hexagon.
I'll take over this, and add a reproducer for an in-tree target!
Sep 20 2018
I think a difference is that the ll composition clash with a user-defined subreg, while the hh composition clashes with a tablegen synthesized subreg.
And we do not get the extra synthesized subreg index for hh when there is a user defined composition.
This patch will conflict with my patch here: https://reviews.llvm.org/D52092
Might conflict with: https://reviews.llvm.org/D51751
Sep 19 2018
I see one problem with this.
Added a reproducer using hexagon as target, that way it was possible to show the difference between having subregister liveness or not.
Fixed some mistakes detected by Florian. Thanks alot for those comments!
Removed a code comment uploaded by mistake in previous diff.