- User Since
- Aug 10 2016, 1:07 PM (61 w, 5 d)
Thu, Oct 12
This patch would be easier to review if you would commit the changes to add new tests and RUN lines separately.
Wed, Oct 11
My only potential concern here is that we could end up blocking optimizations because we're folding to undef rather than zero... but that's probably rare enough that it doesn't matter. LGTM.
The obvious ways to reduce the testcase aren't working for me, and I don't really have a day to sink into it now. If you really need it, it'll probably take me a couple weeks to find the time. (The construct that's triggering it is something like https://bugs.llvm.org//show_bug.cgi?id=34921, but there's some funny interaction GlobalMerge and the exact order of the input which means simple transformations make the problem disappear.)
See also https://bugs.llvm.org//show_bug.cgi?id=34921 .
Tue, Oct 10
Please make sure the title (subject line) for a patch reflects the actual contents of the change, as opposed to referring to Bugzilla; a lot of people skim cfe-commits, so you want to make sure interested reviewers find you patch.
because I believe that it is not uncommon to use i64 indexing expressions in 32-bit code
I tried to work-around the FP issue by creating a constant pool load, but that got a bit ugly - I had to call LowerConstantPool() to create a legal vector of constants.
This doesn't happen with int constants...because int constants are legal but FP constants are not?
The downside of the original version of the patch is most likely worse codesize and runtime performance. If the inliner isn't interleaved correctly, the heuristics become less accurate; the inliner depends on the simplification passes to get rid of dead and redundant code.
Fri, Oct 6
The exact definition of poison is still getting refined, but it's different from undef. undef is a bit-wise property, which is why ComputeKnownBits has to be careful around it. poison works differently; essentially, any arithmetic or logical operation which has poison as an input produces poison, no matter what the other input is. So it doesn't matter what ComputeKnownBits returns for a known poison value.
We should provide both the source and destination VT for any query like this... should be easy to provide even on the IR path.
If I understand correctly, the reason computeKnownBits can't handle this is that it doesn't know what to do with a known poison value? We could just solve the issue in computeKnownBits: currently, it says there are no known bits when it detects a shift overflow, but it could just say, for example, that all the bits are known zero (since the result of computeKnownBits is only meaningful if the value isn't poison).
"it" is more like an instruction prefix than an actual instruction from the perspective of the CPU; it gets decoded into the same thing as an ARM mode conditional instruction. It's not an improvement to replace "it" with an actual instruction.
Thu, Oct 5
Well, in theory there's a crash on trunk that can happen without exotic pointers (something like "gep i64, i64* %x, i128 INT128_MIN" will cause an assertion failure in getSExtValue().)
Yes. Isn't that the point of this patch?
Wed, Oct 4
In my use case, I have 128-bit pointers but "adding" to one of these will only ever modify the lower 64 bits. So for my use case, offsets > 64 bits just don't matter.
Tue, Oct 3
Is it really necessary to have two different of almost identical code to generate an ARMISD::BRCOND? (I would rather have an explicit check for an AND than two versions of the code, if that's the issue.)
Alive is up again.
If we switch to using APInt for offsets, adjustToPointerSize() just goes away, so I don't see how this is forward progress.
Oh, I see, cast<Operator>(U) will crash in cases where the user is something else, like a global variable.
Hang on, there's a more fundamental problem here this is papering over. If your pointers are larger than 64 bits, those pointers can have offsets larger than 64 bits. Since BasicAA is using 64-bit integers to represent pointer offsets, the math in DecomposeGEPExpression will overflow, so you'll get incorrect results, and eventually cause a miscompile.
Mon, Oct 2
Updated to preserve function signatures for EABI functions.
As far as I can see, there are three significant issues with the current -mgeneral-regs-only:
Fri, Sep 29
Thu, Sep 28
Update: https://reviews.llvm.org/rL314435 has now landed, so we can use domtree to detect dead blocks.
These functions would only really compile down to an additional branch.
Wed, Sep 27
Fix CHECK lines.
No, you're right, that's still three instructions; I didn't realize there wasn't a ymm version of vpinsrd.
I meant, "how do we fix x86 in the general case"? Consider the following (with -mtriple=x86_64 -mattr=+xop):
Oops, sorry, lost track of it; I'll commit it today.
Right - I thought about that case, but I couldn't justify it if we need another IR instruction.
ARM/AArch64 are very similar in this respect, since there are multiple vector register sizes. You'll see a similar result for your examples on aarch64. (On 32-bit ARM, we manage to optimize away the extra copy after isel.) I'm not quite sure how much of this logic it makes sense to put into instcombine, given most of the benefit here has to do with the way CPUs split integer and vector registers, but this is probably okay for other targets.
Tue, Sep 26
Yes, this looks right.
See https://reviews.llvm.org/D38299; from the discussion here and my testing, it seems like that patch more closely matches what gcc actually does.
The real problem with computing DT there isn't how long it takes in common cases, it's that the total time will blow up quadratically for large functions.
Mon, Sep 25
Looks reasonable... but missing testcase.
Remove quadratic complexity to calculate DT.
Should I restrict the warning to just redeclarations (without definition) instead?
Fri, Sep 22
x86, arm, and aarch64 have calling conventions which care whether a call is varargs, but not whether a particular argument is specified in the prototype.
Thu, Sep 21
Please make sure to add the mailing list as a subscriber when you post a patch. (I haven't looked at the patch yet.)
Do you want me to commit this for you?