- User Since
- Aug 10 2016, 1:07 PM (157 w, 4 d)
Fri, Aug 16
in the proper order
Address review comments.
Thu, Aug 15
Posted the liveness patch: https://reviews.llvm.org/D66319
Wed, Aug 14
Given that each block only has a single predecessor, there aren't any PHI nodes involved. In that case, what does PHITranslateWithInsertion actually do?
Tue, Aug 13
In the future, please try to ping more frequently than once a month; we don't want to lose patches, even if there are some areas where it takes a while to get review.
If variable A's initializer references variable B, then it will call B's initializer.
Mon, Aug 12
This might be a silly question, but what happens if the initializer for a thread-local variable refers to another thread-local variable? Do you need to initialize both variables? In what order?
The keys are subclasses of llvm::Value, so sorting is hard. There's no obvious comparator to sort global variables. Instructions and basic blocks could be sorted based on DFS numbers, or something like that, but it would be a lot more complicated.
It seems global-isel does not fall back to DAGISel?
Thu, Aug 8
Does this require fast math due to rounding in the sitofp conversion? Off the top of my head, probably it doesn't matter because those cases would produce zero or infinity anyway, but it's worth explaining in the code.
I don't think this is legal in general unless you restrict it to i1. See https://rise4fun.com/Alive/1Hyj .
Okay, makes sense. LGTM
Yes, it's technically a "call", but you don't need the call lowering code. That's dedicated to stuff like passing arguments, returning values, checking whether the function can be tail-called, etc. None of that applies here; the intrinsic always corresponds to exactly one pseudo-instruction, a BL_PUSHLR.
Wed, Aug 7
The approach seems better; I'll comment in more detail on the other review.
This seems better.
Tue, Aug 6
Mon, Aug 5
How did we end up with this testing gap? I haven't been closely watching what tests were committed with previous patches.
introduce quite some code to the target-independent part of SelectionDAG, specifically SelectionDAGBuilder and SelectionDAG classes
For the "=x" thing I was talking about, try the following testcase:
Fri, Aug 2
The changes for vector arithmetic look nice.
I can't think of any issue this would cause, in general; following frame records should still work the same way, and DWARF unwinding should still work.
Can you remove the dead MachineDominatorTree *MDT; declaration?
I think this is semantically the correct fix, but I have some concern about the way the APIs are written.
It would be cleaner to convert this test to FileCheck, but it's probably not worth spending the time at this point.
Thu, Aug 1
Sorry about the delayed response.
Maybe you can make a simpler MIR testcase? (http://llvm.org/docs/MIRLangRef.html)
This only affects inline asm, it looks like, not general instructions? (I looked briefly, and it looks like instructions go through AArch64InstPrinter::printOperand instead.)
Wed, Jul 31
Okay, delaying some of the work to sense, since you can't really modify FunctionChain while you're iterating over it.
I don't think there's any way to define shouldSignExtendTypeInLibCall that completely solves the issue without target-independent changes. Yes, it's true that floats should not be zero-extended, but they also shouldn't be sign-extended. So makeLibCall should do something equivalent to .setSExtResult(false).setZExtResult(false) for the result, and Entry.IsSExt = false; Entry.IsZExt = false; for the arguments. And this needs to apply only to float arguments/results; integer arguments and results need to be sign-extended from 32 bits to 64 bits. (Not sure we currently generate any libcalls with 32-bit arguments/results on RV64, but that could change in the future.)
this looks like it could be a Core Issue
Tue, Jul 30
Would this also be profitable for Thumb2?
Updated heuristic to match the underlying encodings a bit more closely.
Here, if I use F->erase(TBB), the memory leak error is still existed.
If we're going to recognize a C function that returns an aggregate, we need to be very careful that the form we're recognizing is a specific, known pattern. We need to be sure that we're recognizing a specific way of returning an aggregate, and TLI needs an API to tell transforms which specific way it recognized. We don't need to add all the possible patterns in the first patch, of course.
Mon, Jul 29
You probably want F->erase(TBB), which both removes TBB from the list of blocks in the function, and deallocates TBB. I guess the empty block with no predecessors doesn't really matter much, in the long run, but easier to understand if the transform cleans up after itself properly.
Fri, Jul 26
Please rebase against trunk.
Thu, Jul 25
Is there any reason to expect that unreachable loops have exactly one block?
LGTM with one nit.
Wed, Jul 24
I don't think you actually need a recursion guard; you only recurse on ConstantAggregates, and they can't refer to themselves circularly. But maybe it's a good idea anyway to avoid exponential compile-time in certain edge cases.
Any reason why not to use this for thumb2 as well?
Oh, okay, that makes more sense.
Do you think this is blocking for this fix to get in?
LGTM with one minor change.
I suppose the reg scavenger can also spill it if it really can't find another register, but I'm not sure we can do much about it at that point.
I'm not sure this fix is actually solving a real issue, as opposed to merely hiding the issue for the given testcase.
Tue, Jul 23
I'm not sure this is something we can reasonably enforce without making big changes in other areas of the compiler. For example, we call eraseFromParent() all over the place, and some large fraction of those calls could eliminate the last use of a blockaddress.