- User Since
- Aug 10 2016, 1:07 PM (114 w, 1 d)
Wed, Oct 17
I think this is looking good; hopefully Reid will get a chance to look briefly to make sure this matches what he was expecting.
Please don't wait months to ping on a patch; if you haven't gotten any response in a week, please ping, and if you haven't gotten any response in a couple weeks, please email llvmdev. We want to review patches in a timely fashion, but sometimes reviewers miss an email, or incorrectly expect someone else will review the patch.
This analysis seems very fragile.
The approach is fine.
I think you have to be careful about something like the following:
It looks like you didn't touch LibCallSimplifier::optimizeLog, but I guess that's not necessary (it probably shouldn't be using emitUnaryFloatFnCall anyway).
Starting multiple threads on the LLVM mailing lists with the same message is spam. Please don't do that.
Tue, Oct 16
I am not sure what you mean by freezing the pointer I am afraid.
Took another shot at the wording. This should be a bit more explicit about what we're assuming, and what transforms are allowed.
I think we still need the clang patch to preemptively strip nonnull annotations from C library functions before we turn on EnableNonnullArgPropagation by default, to be safe. But yes, the right approach is to add the correct nonnull attributes, then enable nonnull propagation.
I'd like to see more test coverage for this, I think. If I'm following correctly, this should affect something like the following on AArch64?
Looks like the latest update is missing a test file?
This doesn't look like the patch I was expecting.
Please make sure your patches are reviewed by a qualified reviewer before you merge.
Mon, Oct 15
This probably needs at least a few testcases... not so much to duplicate the tests you're going to add for exception handling emission, but rather to make sure the error handling works correctly.
Fri, Oct 12
LGTM with a couple more tests with an explicit AND (to show the pattern triggers for "and i32 %shamt, 31", but not "and i32 %shamt, 15").
I'm not sure it makes sense to put this information in the DataLayout.
Reverted in r344405.
Fixed signature of __aeabi_uwrite4/8.
The packed/may_alias struct pattern is the same pattern we use in the x86 immintrin.h for unaligned loads; should do the right thing in general.
lib/Transforms/Scalar/Sink.cpp is probably a good starting point
llvmdev thread is titled "Volatile and Inserted Loads/Stores on MMIO".
Thu, Oct 11
You're making the lattice really confusing. Essentially, there are now two different merging rules: one is used if the caller calls mergeInValue, and a different one is used if the caller calls markNotConstant etc. So it's not obvious what the lattice actually represents.
What happened to this patch?
Anyone have any thoughts about potential security implications for this? Normally, I wouldn't really care about assuming code never has undefined behavior, but an indirect jump to an arbitrary address is much easier to exploit than other sorts of undefined behavior.
Wed, Oct 10
Missing Sema changes?
Probably should have a test for something like float x = (__uint128_t)-1;, to make sure we print something reasonable.
I don't think anything will go wrong in this case, specifically... I'm more generally worried about the keeping a list of float routines in the MIPS backend which is essentially copy-pasted from RuntimeLibcalls.def.
Mon, Oct 8
I was hoping there was an opportunity here to avoid the need for backend-specific peepholes
I guess that sentence was weird. I meant, if the shift amount is too large, DAGCombine will fold the shift to undef.
Found as a build failure testing a new compiler on Android.
Would it also make sense to enforce the corresponding restriction, that non-tail-call indirect branches shouldn't use x16/x17?
You can't ANY_EXTEND here; that will confuse DAGCombine, which will, for example, fold shifts where the shift amount is too large to undef. Other targets normally deal with this at isel time; see, for example, MaskedShiftAmountPats in lib/Target/X86/X86InstrCompiler.td .
I don't think that was the approach @rsmith was suggesting; you don't need to wrap every possible kind of expression. Rather, at the point where the expression is required to be constant, add a single expression which wraps the entire expression tree to indicate that. So for "constexpr int c = 10+2;", the expression tree for the initializer is something like ConstExpr(BinaryOperator(IntegerLiteral, IntegerLiteral)).
Fri, Oct 5
Thu, Oct 4
How are you testing?
Needs a testcase for the error message like we have for other intrinsics in test/Sema/builtins-arm64.c . Otherwise LGTM.
Code changes seem fine.
Wed, Oct 3
read_register only allows reading reserved registers because reading an allocatable register is meaningless (the compiler can store arbitrary data in allocatable registers).
How come this hasn't been an issue for those targets up until now?
This has no impact by itself that I've been able to see, as LICM typically doesn't see such phis as they will have been converted into selects by the time LICM is run
Every Integer is representable (lossy of course) as a float as far as I know.
You could just directly test that the computed memory operand is correct: write a test that runs "llc -stop-after=isel" and check the MIR. Without your patch, you should see something like "ST1Fourv2d killed %5, %4 :: (store 48 into %ir.addr, align 64)"; with your patch, that will be "store 64".
The Arm MaxInstLength could be increased to 6
Tue, Oct 2
The commit message should refer to https://bugs.llvm.org/show_bug.cgi?id=31141 (assuming this patch fixes that bug).
unless we count that they are 32bit instructions? Do we do a lot of modelling of that?
I'm kind of confused by what you're doing here... fmaxl is a C library function; C code is allowed to take its address and call it indirectly. So changing the lowering based on the name of the callee seems wrong.