- User Since
- Oct 27 2012, 6:35 AM (307 w, 6 d)
Uhm, after thinking a bit more, i think we'd only care about ADD? I haven't really thought about it though.
But in ADC case, i guess the same as with ADD, but prefixed with CLC -> no point in using ADC in the first place.
Ok, sounds good. At worst, this can be extended.
I'm not sure why there is no x86 test coverage.
Won't ADC work for this purpose?
ADD evaluates the result for both signed and unsigned integer operands and sets the CF and OF flags to indicate a carry (overflow) in the signed or unsigned result, respectively, too.
Some plumbing missing?
You can always write an unit-test, too.
Better, but these blacklists need to be config options, not random hardcoded globs.
See e.g. google-runtime-references.WhiteListTypes.
I believe i will have to move BZHI selection from tablegen into here, due to https://reviews.llvm.org/D48768#inline-461084,
else we will have test coverage issues, and code duplication.
Do we somehow enforce that in %r = select i1 %c, i32 -1, i32 %a, -1 is in the middle?
If not, we miss at least one case i think:
$ ~/src/alive/alive.py /tmp/test.txt ---------------------------------------- Optimization: 1 Precondition: true %a = add i32 %x, 42 %c = icmp ugt i32 %x, -43 %r = select i1 %c, i32 -1, i32 %a => %c2 = icmp ult i32 %x, -43 %r = select i1 %c2, i32 %a, i32 -1
I'm somewhat sure this kind of change needs it's own tests, outside of $backend.
If it no longer crashes, i guess you can test for that?
I'm gonna stop here, i think this is the state for the review.
And get rid of a last few unneeded movzbl.
This looks questionable to me.
I don't disagree with the reasons stated about llvm types.
But is that *always* true?
I would personally be very surprized, and consider this a false-positive.
Use ISD::ANY_EXTEND. This gets rid of the extra movzbl of the NBits.
At least that is what D48768 already produced.
If NBits is larger than 64 (in 64-bit case), the shift was already undefined, i think?
Unless there is a need to conditionally use the one-use variant,
two separate functions seem cleaner. But no strong opinion here.
TBM BEXTRI is of no interest to us here.
One last rebase
Wed, Sep 19
What i think is missing here, is that
The code incorrectly inferred that the relationship of a constant expression X to itself is FCMP_OEQ (ordered and equal), when it's actually FCMP_UEQ (unordered or equal).
simply states it as the matter of a fact.
Clearly, since this patch is here, the previous choice was wrong (i guess?).
So some more blurb with explanation as to why it is so would be good.
(i'm guessing nan / inf etc?)
LG with a nit.
Tue, Sep 18
@vsk thanks for taking a look!
@rsmith - ping.
This one should be rather uncontroversial i think? Is this moving in the direction you suggested? :)
Mon, Sep 17
Great. I was worrying i was missing something obvious here.
Would be great if this could go upstream first, and then backported.
Are there tests with comparisons with nullptr? (Also mind the nullptr is valid thingy)
Do keep in mind that there is more than one backend, more than one target architecture.
Sun, Sep 16
Do we care whether the *entire* old patter goes away, or just one more instruction?
But yes, should work, let's see.
Just to reiterate, i don't see what else we can do here.
We do need to emit two instructions.
And i do think we should indeed be emitting bextr in these situations.
Sat, Sep 15
found some occurrences of this expression on our code base.
I don't think this makes much sense.
This looks weird.
It will now completely skip all the explicitly instantiated functions, no?
I'd think the problem that needs to be fixed is that it considers the local variable int a_template; to be in the function argument list.
Thank you for the review!
Hm, so it wasn't a sanitizer buildbot being flaky as usual.
Sorry for the trouble and thanks for the fix :)
Filed the ones that i can pinpoint, marked them as blockers of PR38890
Fri, Sep 14
Thank you for working on this!
Are you sure you shouldn't be fixing this in the backend?
- Should this be somehow more parametrized, rather than hardcoding magical Size > 4? Some target info?
- Is there some reverse transform, that tries to actually for memcpy from such expanded load+store pairs? Shoukd it e disabled too, to avoid looping?
Thu, Sep 13
And a follow-up in rL342182.
Yes, please :) Else, the message seems a bit too empty.
I don't think it should point (via NOTE:) at the each decl though.
I think i lost track, was there some consensus on this 'support for m680x0' somewhere on the mailing list?
I don't dis-welcome this, just asking.
The tests seem to have disappeared form the diff.
I'm not sure.
I think we really should have two separate matches, and two switch()es.
Else i think we may use the wrong predicate..
Also, i *think* i will add one/two patterns to this new matcher (less canonical variants with extra uses),
so specifying the pattern twice seems sub-par.