In D61773#1504714, @asb wrote:

Better to modify the tests to add nounwind. I have a patch that does that sitting around (I added CFI directives downstream but didn't clean it up for submission). Let me apply the test suite patch upstream, then can you rebase on that.

Better to modify the tests to add nounwind. I have a patch that does that sitting around (I added CFI directives downstream but didn't clean it up for submission). Let me apply the test suite patch upstream, then can you rebase on that.

Update:

• Expanded and improved tests
• Set ABI defines
• Remove errant TODO
• Use alignTo

Yes, I think leaving the current behaviour probably makes sense. I encountered that test failure too and mask that test on the basis that that those builtins aren't documented as being guaranteed to work https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html (that page even notes that crashing is allowable behaviour).

The description of the llvm.frameaddress and llvm.returnaddress intrinsics seems to indicate that these are "best effort" and LLVM doesn't really guarantee a correct result for a depth > 1 https://llvm.org/docs/LangRef.html#llvm-returnaddress-intrinsic https://llvm.org/docs/LangRef.html#llvm-returnaddress-intrinsic

LGTM!

Some initial feedback:

• This patch is pretty huge which makes it pretty hard to meaningfully review
• There seem to be effectively no tests. I'd expect test/CodeGen/Connex and test/MC/Connex to be reasonably well populated
• Plenty of code commented out or with date-based comments that don't match our style, e.g. // 2018_*
• Files have the old license header rather than the new one

Thanks for the report Roger. I agree with @jrtc27's analysis, it makes sense to limit requiresDiffExpressionRelocations to returning true only for code sections.

In D60294#1462628, @spatel wrote:

Not sure if it's relevant to what this patch is trying to do, but x86 has a similar example described here:
https://bugs.llvm.org/show_bug.cgi?id=24447

Good catch Roger, thanks! This looks good to me, though I wonder if it's worth being a bit more explicit in the checkPseudoAddTPRel comment.

With Eli's suggested change (sinking the assignment to the ADD/SUB branches), this seems good to me.

This looks good to me. The GCC torture suite is happy with this code too, which serves as an extra (but non-exhaustive) sanity check. It's not immediately obvious to me that checking for OptForMinSize is better than OptForSize, but the precise criteria for when something should be done at Oz vs Os don't seem to be well specified and checking OptForMinSize is consistent with other backends, so the right choice for now.

In D60294#1456609, @efriedma wrote:

Given that we're splitting the GEPs before SelectionDAG, the only way to preserve the optimization is to avoid re-merging them... and I can't think of any reasonable approach for that other than something along the lines of this patch. Well, I guess we could just completely disable folding without regard for whether the addressing mode is legal, but that probably interacts badly with type legalization.

The alternative, as you note, is to do something after isel. Arguably that would be more effective. At that point you have a better idea of what constants are actually necessary, and you could integrate it with other similar optimizations like optimizing related integer constants. This is sort of along the lines of RISCVMergeBaseOffset.

Many thanks for this. I've added some minor formatting and comment phrasing nits, but otherwise this is looking good to me.

LGTM, thanks!

This got missed somehow as I had a functionally identical patch in my local development tree (though not with as thorough tests - thanks for that!). LGTM, thanks Ed.

Thanks Roger, this looks good to me.

Hi Lewis, this looks good to me - thanks! Do you want to request commit access as per the developer policy so you can commit yourself?

Looks good to me, thanks.

Thanks Lewis, this looks good to me. We should later revisit whether we should accept call 1234 like GNU as does.

LGTM, thanks.

I think it would be worth rounding out the tests a bit, especially as it should just require a trivial amount of copy and paste and editing to do so:

• ashr64_minsize
• shl64_minsize
• lshr128
• ashr128

LGTM, thanks!

@rogfer01: Thanks for the review. You're right that we could do with coverage for varargs under the hard float ABIs. I've modifed test/CodeGen/RISCV/vararg.ll so it provides common coverage for vararg passing for these ABIs (which of course should be the same as the integer ABIs). How does this patch look now?

Thanks for the review @shiva0217 - how is this?

Hi Daliang., thanks for the submission. One problem I think you'll find once you extend the tests is that support for realigning the stack is going to be required - e.g. if passing a double (8-byte aligned) on the stack (4-byte aligned).

LGTM, thanks. But would be better titled to reflect that it include GPR != simm12 optimisation. Maybe even better titled as "[RISCV] Improve codegen for icmp {ne,eq} with a constant" or similar (IMHO!)

Thanks, LGTM.

In D59233#1439656, @hubert.reinterpretcast wrote:

Thanks @xingxue for the review. I will update for the error handling before committing.

This looks good to me, and given @rogfer01 indicated he's also happy, I think it's ready to merge. Thanks Luis!

Looks good to me, thanks.

Thanks for this Luis. This needs coverage for the optForMinSize checks. I'd recommend extending shifts.ll with minsize versions of the tests, e.g.

Added a few minor nits. You'll want to add a test for debug info to select-optimize-multiple.mir. I had a quick play with this and https://reviews.llvm.org/P8135 looks like a sensible way to do it.