Ping?

In D46324#1096882, @kparzysz wrote:

One thought I had was: if there is a block with only an unconditional branch, should we fold it away first? That is, redirect any branches to that block to the target of the branch?

Add some comments

Ping?

Add lane index range check

In D45878#1081950, @jonpa wrote:

In D45878#1079514, @gberry wrote:

I started to write the utility functions in MachineInstr/MachineBasicBlock, but thinking about it, I wonder if it wouldn't be even better to integrate fully the handling of DBG_VALUEs and DebugLocs into splice()? After all, the DBG_VALUEs should *always* be (re)moved together with the MachineInstr, so why expose this to a particular optimizer pass? If splice() would do this per default, the optimizer would not have to worry about it, and could not even forget about it. Is there a reason to always do this on the side?

If this is guarded by a check to see if the following instruction is a DBG_VALUE, I think builds without debug info would not suffer from increased compile-time. BTW, is there a better way to check if it is a debug build or not, like maybe hasDebugInfo()?

One issue with this is that the caller usually has an iterator, that now gets invalidated if a DBG_VALUE is moved automatically. I think this means splice() has to return an iterator to the next following instruction after the moved one(s). There would also have to be one method that handled reverse iterators. My idea is that splice would only do this if an iterator is passed as an optional argument, so that optimizers could gradually get used to this.

Again, I am not an expert here, just assuming from quick observation that when moving an instruction it is good to

1. move any connected DBG_VALUEs with the moved MI
2. update the DebugLoc of MI for its new position.

I have so far just reused the code in MachineSink, but I don't know how good this is or if it could be improved. MachineSink is currently checking through all DBG_VALUEs after MI and picking only the right ones, and stopping at first non-debug MI. This makes sense to me, even though I'm not really clear on why the DBG_VALUEs could end up mixed like that. Could it perhaps be that if we started handling this properly everywhere, we could assume that they are never mixed (that DBG_VALUE that express a use of a register is always to be found directly after the defining MI)? Does this relate perhaps to instructions with multiple defs (like an address post-increment), and if so, should we perhaps scan MI for all explicit defs instead of just the one at index 0?

Of course, Pre-RA this could be done with MRI use-def information instead of the search.

Then there is the question of what to do when moving a range of instructions. Depending on how mixed up the DBG_VALUEs might get (when all optimizers are doing the right thing), one might want to either handle the whole range of instructions to find out what following DBG_VALUEs to also move, or perhaps just look at the last instruction of the range.

Similarly, eraseFromParent() could always also erase any connected DBG_VALUes. There is actually eraseFromParentAndMarkDBGValuesForRemoval, but only with virtual registers, sofar. Should the search for DBG_VALUES after MI be added here to be done post-RA?

I would like to see some clear comment on exactly what is expected from an optimization pass that moves / erases instructions in regards to DBG_VALUE instructions, but can't find one...?

Do you think this makes sense, or should we stick with separate utility functions?

In D45858#1077153, @mgrang wrote:

A quick background on the reverse-iteration bot: https://llvm.org/docs/CodingStandards.html#beware-of-non-determinism-due-to-ordering-of-pointers
Unordered containers like SmallPtrSet and DenseMap do not guarantee the order of iteration of elements. So iterating them can result in non-deterministic codegen. However, not all instances of iterating an unordered container result in non-determinism. Given this, it becomes difficult to identify which iteration instances can cause this behavior. The reverse iteration bot iterates such containers in reverse, by default, to weed out such cases.

However, the bot is only useful only if there are tests which stress/depend on iteration order. And while it may be possible to write such tests I am not sure if it's worth the effort. My experience also has been that I have randomly run into non-deterministic behavior and debugging led me to problems in iteration order.

I've been looking into this issue for AArch64 recently as well (see bug 37240 for a PostRA scheduler issue I found). I'll be filing more bugs and/or posting fixes for other cases that we hit on our target with our different compiler flags in the next few days.

I've put the full diff for my alternative fix here: D46063

In D45858#1075784, @mgrang wrote:

If there is a unit test for this then it should have been uncovered in the reverse iteration bot: http://lab.llvm.org:8011/builders/reverse-iteration

In D45858#1073565, @spatel wrote:

That looks ok, but I'm curious;

1. How do you discover this problem?

In D45770#1072174, @paulwalker-arm wrote:

In D45770#1071047, @gberry wrote:

Ugh, good find. Isn't this an issue if variable sized stack objects are present as well?

The graphic (within the same file) showing the stack layout suggests a base pointer is used in such circumstances, so my guess is no.

@MatzeB This isn't about the scavenging of a register to use for large offsets, etc., but rather the scavenging of stack slots in the CSR save area to use for regular stack objects.

Ugh, good find. Isn't this an issue if variable sized stack objects are present as well?

Another thought, would it be worthwhile to add a serialized MIR function property for this usesRedZone flag so you could write MIR tests for this instead?

In D39976#1049844, @evandro wrote:

In D39976#1040345, @gberry wrote:

In D39976#1027478, @evandro wrote:

FeatureSlowPaired128 was just too coarse. The alternative would be to change it to something more specific, like FeatureSlowSomePaired128Sometimes, and then create yet another when for the next generation to specialize it further. Instead, querying the scheduling model seems to be a much more reasonable approach.

I'm more confused now. 'FeatureSlowPaired128' controls whether certain load/store opcodes are combined to form paired load/stores. But this change prevents some load/store opcodes from having their base register increment folded in. The two seem unrelated.

This change is more generic and flexible than FeatureSlowPaired128. This change controls not only when loads and stores are paired, but also other foldings that this pass performs, including the pre or post indexing of the offset register.

Add test

I get it now. LGTM

I'm not sure I see what's wrong with removing the second COPY in either of these cases, unless the semantics of the multi-register COPY are that each component register is copied in order?

In D39976#1027478, @evandro wrote:

In D39976#1027475, @gberry wrote:

In D39976#1017864, @evandro wrote:

Methinks that the gist is to move away from features and to rely more on the cost model. In the case of this patch, it also removes the feature FeatureSlowPaired128 in D40107.

That seems like a worthwhile goal, but this change doesn't really seem to be accomplishing that. If the sched model is being used by a subtarget-specific heuristic, that seems like just a more roundabout way of achieving the same result for your subtarget. Is there any net effect of this change combined with D40107?

FeatureSlowPaired128 was just too coarse. The alternative would be to change it to something more specific, like FeatureSlowSomePaired128Sometimes, and then create yet another when for the next generation to specialize it further. Instead, querying the scheduling model seems to be a much more reasonable approach.

In D39976#1017864, @evandro wrote:

In D39976#1017321, @gberry wrote:

Would it not be simpler to just add a subtarget bool that controls whether the problematic opcodes are emitted and set it for your subtargets (similar to the way STRQroIsSlow is handled)? That way you could avoid generating them not just in this pass, but in ISel and frame lowering?

Methinks that the gist is to move away from features and to rely more on the cost model. In the case of this patch, it also removes the feature FeatureSlowPaired128 in D40107.

Would it not be simpler to just add a subtarget bool that controls whether the problematic opcodes are emitted and set it for your subtargets (similar to the way STRQroIsSlow is handled)? That way you could avoid generating them not just in this pass, but in ISel and frame lowering?

Add test/CodeGen/AMDGPU/postra-norename.mir as requested by Quentin

@arsenm Could you take a look at the AMDGPU change?

New version based on review feedback.

Checking hasExtraRegAllocReg in isRenamable sounds reasonable to me (though it may require the interface to be changed slightly since we'll need the opcode). An alternative that is closer to what I initially proposed would be to have setReg and addOperand both clear the isRenamable flag. This way the only operands that would be renamable are those that have been untouched since they were marked renamable by the register allocator. This would also allow us to get rid of the extra code that I added to clear the renamable flag in some post-RA target code. Targets would be able to opt in to preserving the renamable flag by correctly preserving the renamable flag in post-RA transformed code, but by default the flag would be set conservatively. I'm going to try this approach out to see what it looks like and what kind of impact it has unless someone has a reason they think it won't work.

I've thought about this some more and tested it out on Falkor. As currently written this change causes SIMD store instructions to not have pre/post increments folded into them, causing minor performance regressions. I have the following general reservations as well:

• does using the max latency of the load/store and add make sense given that the operations are dependent?
• does always favoring latency over number of uops (an approximation of throughput) make sense? unless the operation is on the critical path I would think not.

Add comments to address feedback from Quentin

D42449 adds the machine verifier check that no renamable operands are assigned to reserved registers

I've uploaded a new version that addresses most of the issues brought up by @qcolombet.

Updates based on Quentin's comments

Thanks!

Ping?

@tstellar could you specifically take a look at the AMDGPU backend changes?

I'm not sure using the redzone is safe on all targets.

Okay, I get it now, I had the wrong case in mind. I was thinking that you could put the scavenge slot close to [sp], but that doesn't work since you have a large outgoing stack parameter area.
This change looks good to me, but you might want to get a second opinion since my thinking on this hasn't been that clear.

I think what is bothering me about this change is that the return value of hasFP() now seems more dynamic. Did you consider a potentially simpler fix of just creating the spill slot as a fixed stack object with a hard-coded offset that would guarantee it is directly addressable from the SP/FP?

I'll resubmit this for review if I ever get back around to it

In D40876#958683, @gberry wrote:

One thing I noticed when looking at this: it seems fragile to me to have MachineFrameInfo::computeMaxCallFrameSize() and PEI::calculateFrameInfo() doing essentially the same calculation with duplicated code.
Would it make sense to add an assert to PEI::calculateFrameInfo that checks that the previously calculated MaxCallFrameSize isn't smaller than the one calculated later?

One thing I noticed when looking at this: it seems fragile to me to have MachineFrameInfo::computeMaxCallFrameSize() and PEI::calculateFrameInfo() doing essentially the same calculation with duplicated code.
Would it make sense to add an assert to PEI::calculateFrameInfo that checks that the previously calculated MaxCallFrameSize isn't smaller than the one calculated later?