- User Since
- Jan 23 2016, 8:31 AM (243 w, 12 h)
Thu, Sep 3
Does run-twice free the module? I’ll have to look for the repro again but I was only able to observe a crash when the module is free’d and it doesn’t even require running the pass again...
Mon, Aug 31
Actually I don't think this is correct so I'm closing this. It should actually be valid to only mark a load as constant even if it is only true behind a branch and in such case moving the load out of the branch should indeed have the metadata stripped out.
Other frontends could. I'm adding this because TBAA can already use this but right now if the frontend emits them, it can still be stripped out by instcombine.
Sat, Aug 29
Propagate alias info to load and store instructions in additional to function calls.
I'm currently running the existing tests to find regressions... I'll probably just add tbaa to most/all of the existing tests and it'll probably take a while...
Aug 8 2020
Jul 22 2020
Seems that this wasn't automatically closed by the two commits so I'm closing manually...
Jul 17 2020
OK, so I guess at least R_X86_64_PC32 should be fixed too. If you don't mind, feel free to commit an updated version of this. I can also update this later next week after I'm free again, though, as I said, I have a hard time constructing an IR test for this....
Is there a difference between R_PPC64_REL32 vs R_AARCH64_PREL32 vs R_X86_64_PC32? I didn't add mask because the x86 one doesn't have it.
Jul 14 2020
Nov 2 2017
Oct 30 2017
Updated the test to match the format after autoupgrade.
Actually I just realized that the last test is referring to auto upgraded attribute numbers, I'll check if I can directly use new attribute syntax and have more predictable numbers (or do regex matching instead)
Oct 26 2017
Do you need this right away, or can we wait for that change to proceed?
Oct 25 2017
Thanks for the review. Test removed and patch updated. I'll merge some time tomorrow.
Oct 24 2017
Who should make the decision of whether the test should be included or if it's needed? Or how much longer do I still need to wait before merging either with or without the test?
Oct 22 2017
Thanks for the review.
Oct 20 2017
Oct 17 2017
Oct 16 2017
Thanks for review.
But the point that SM does not do that makes me thinking that it might be a separate patch.
Oct 14 2017
Thanks for the review.
I'm hoping for input from a code owner to see whether they think introducing a UnitTest is worth it.
Oct 12 2017
Personally I would be happy with the change without a heavyweight test.
Oct 11 2017
I put together a really simple test based on the only other occurance of streamer in the tests (from DWARF tests). It reliably crashes without the patch and doesn't with the patch. This is about as much as what I can possibly come up with. A similar test can be added for AArch64 but won't be testing anything. I have no idea what result I should test.
Oct 9 2017
I've found another failure in instcombine (see last test) so it seems to me that CastInst::isBitOrNoopPointerCastable is the right place for this logic. This also answers my second question since the cast between address spaces is already handled by isBitCastable
Oct 5 2017
we might want at least an assert here.
My guess is that the only way to plausibly test this may be to add a new unittest maybe under unittest/MC?
One thing I'm a bit puzzled about in D30724 is that line 467 (in review) looks like it will move a LastEMSInfo unique_ptr<ElfMappingSymbolInfo> that points to nullptr on first call as the class member on line 686 isn't initialized. This may or may not be important and I may have missed something in the code.
Why do you need to call this here and in reset()?
Sep 27 2017
There was no comment since the only comment I can add would be repeating the condition itself. I do like to add comment about why this is needed or if this is the only way we can fix the issue but that's also what I'm not sure about (quesiton 1 in my review comment). Anyway, I added a comment to repeat the condition for now.
Sep 26 2017
Jul 29 2017
Use AccessTy and added a test to make sure that the case where we load from an non-integral point is still optimized.
Jun 21 2017
I see the non-arch specific property as the good part since currently everything other than PPC claims to support large model with PIC and just generate wrong code. If a fallback that's always implementable and correct is defined, the only arch-specific changes needed will be for efficiency and not for correctness.
If the jump table is writable
Jun 20 2017
Why would a writable jump table help?
Who is we? I'm moderately sure that is not the case in general, since it would create less efficient code.
PPC changes reverted and tests updated. I believe this addresses all the comment so far.
the existing 32-bit offsets are probably fine.
It puts offsets between the BB and a picbase into the jumptable.
I expect something better could be done for PPC, but this is entirely in line with the existing 32-bit code and correctness comes before performance. I pretty strongly object to characterising the patch as "wrong".
Jun 18 2017
Jun 17 2017
I'm seeing a similar failure caused by the same code even with this patch applied on LLVM 4.0.
Jun 15 2017
Jun 10 2017
Oh and this fixes https://bugs.llvm.org/show_bug.cgi?id=33345
Jan 26 2017
Jan 10 2017
I would expect an IR test for this,
Except you don't?
Rebased to only include the code model part....... Hopefully this time someone can comment on the whole PR instead of half at a time ;-p........
...... Looks like the relocation part was implemented in a later pull request https://reviews.llvm.org/D28122 that's already committed.........................
Jan 5 2017
(i.e. can both llvm and gnu libunwind handle DW_EH_PE_sdata8)
Dec 24 2016
Dec 15 2016
Dec 14 2016
Tests added. Also discovered and had to fix two other bugs since the arch detection was wrong for aarch64_be ELF file....
(and add the required tests please)
Dec 13 2016
Use target endian for data relocation. This one actually depends on https://reviews.llvm.org/D27609 now....
Use target endian for data relocation.
Dec 11 2016
Dec 9 2016
Tests added and endianess issue fixed.