@aditya_nandakumar I've added a target hook that defaults to enabling this.

This seems like something that most targets would want done by default. I wonder if we should have a standard set of legalizer actions that targets can use. LGTM anyway.

In D51953#1237061, @aditya_nandakumar wrote:

Hi Amara - would it be possible to add this as part of AArch64 pass pipeline? Adding this to all targets would result in needless burning of compile time for targets that don't need this.

This isn't really ideal because of how trivial it is currently. The issue is that if we added the capability to InstSimplify (which is where it would fit most naturally), we'd still end up with the problem of running the whole of InstSimplify in the GlobalISel pipeline. It's reasonable for now I think so I'll commit with the changes requested.

Looks ok, if a bit cumbersome with the additional state (do we need to hold references to it?). I'd like to see how this state is used with the CSE builder first though.

Ok, I'm surprised that would be a realistic scenario but I'll make the change. Thanks.

You're right, that's a simplified expression.

Ping.

Hi Quentin,

In D51145#1224873, @spatel wrote:

In D51145#1223770, @mcberg2017 wrote:

I added one more non fmf instruction and there is room to add others if needed.

There's also discussion about the definition of FPMathOperator and the relation to FMF here:
https://bugs.llvm.org/show_bug.cgi?id=38086
D51646

Ok, to me it seems like instructions for which fast math flags can't apply shouldn't be classed as FPMathOperators. insertelement/extractelement are just moving raw bits. If we disallowed those instruction types in the FPMathOperator classof() then this problem should go away, and we don't incorrectly relax FP semantics like in the case I pointed out.

I’m not seeing how FP instructions can not carry flags. The IR semantics are defined to be strict FP unless explicitly relaxed.

In D51362#1221028, @qcolombet wrote:

It can't do that all the time, with these G_ICMPs for example it also has to swap the predicate (not that we even have imported patterns that could possibly match for AArch64).

Why not?
I understand G_ICMPs are special, but we could come up with whatever complicated logic in TableGen.

Hit submit too early:
So with the above code B will not have the flag cleared? If that's the case it doesn't seem right to me.

So with this change, if you have:

So overall I think we should have some form of IR canonicalization before translation for the majority of cases to be handled for -O0.

In D51362#1219573, @qcolombet wrote:

Going back to a higher-level description for all that stuff, I wonder if ISel could already handle this.

Indeed, it seems easy, at least conceptually, to teach tablegen to check for the commuted patterns to get the immediate version whenever possible before trying the non-immediate variant.

It can't do that all the time, with these G_ICMPs for example it also has to swap the predicate (not that we even have imported patterns that could possibly match for AArch64).

In D51362#1218451, @rtereshin wrote:

In D51362#1218394, @aemerson wrote:

I'm not against a separate IR canonicalization stage, even though it's a bit overkill for this particular case. At the moment it looks like InstCombine is doing the canonicalizaton for this case, so re-running that is out of the question. A new pass looks to be on the cards. Anyone else have opinions on this?

Does that mean that something after the last InstCombine produces icmp's with the non-canonical operand order? If so maybe that something needs to be chased down and fixed instead.

I'm not against a separate IR canonicalization stage, even though it's a bit overkill for this particular case. At the moment it looks like InstCombine is doing the canonicalizaton for this case, so re-running that is out of the question. A new pass looks to be on the cards. Anyone else have opinions on this?

In D51362#1216865, @qcolombet wrote:

Hi Amara,

That patch worries me, because I feel that we are going to pull all the LLVM IR code that does canonicalization.
The way I see it, is the input IR should already be in a canonical form and thus we don't have to do that.

If that's not the case, I would argue that bad output code is fine (garbage in, garbage out).

What do you think?

Cheers,
-Quentin

I'd like to get this in. LGTM but needs one issue addressed.

In D51197#1215516, @igorb wrote:

In D51197#1215499, @aemerson wrote:

x86 is still using UADDE, and it's generated only by IRTranslator, so if you do this then x86 will lose support for compiling llvm.uadd.with.overflow intrinsics. @igorb what do you think?

Which is exposing a hole in our general GISel testing, where individual changes in passes can result in overall support for some input IR to be lost.

Hi,
I think currently X86 use G_UADDE generated by Legalizer to support 64bit add on 32bit architecture.

Regards,
Igor

x86 is still using UADDE, and it's generated only by IRTranslator, so if you do this then x86 will lose support for compiling llvm.uadd.with.overflow intrinsics. @igorb what do you think?

Looks like Lang added the original code, but LGTM anyway. I even think we did this exact change downstream a few years ago at ARM...

Sure, done.

I'll take over and fix this in the way I suggested. Thanks for reporting this!

I'm a little uneasy about specifying libm in the names of the intrinsics. The langref mentions libm in the description, but the semantics don't exactly match due to ignoring errno. That said, I don't really have a suggestion for a better name, so the only thing I can ask is that we make it a bit more explicit in the documentation what we mean by "LIBM" here.

Thinking about this more, I can't think of any reason why we'd ever want to add offsets to an existing offset list. It would probably be better if valueIsSpit cleared the vector (and the doc comment made this clear).

Could you upload with more context please, and a test case?

Thanks for taking this on. I think some x86 tests would also be good here as there evidently isn't much coverage at the moment.

Hi Daniel, sorry for the delay.

LGTM.

New patch now only zero-extends for stores, removing the use of getBooleanContents.

I'll re-do this patch to unconditionally zero-extend for returns and also fix up the G_STORE legalisation to always zero-extend too. Given we don't have any target hooks that give us the information we need, let's go with just matching SelectionDAG behaviour?

I got my branches mixed up, this patch is missing test updates. Will upload a new one soon.

Overall I'm in favour of this change, so LGTM.

Hi Craig,

Looks like this change caused http://green.lab.llvm.org/green/job/test-suite-verify-machineinstrs-aarch64-O0-g/1944/ as well as others.

In D46889#1104823, @aprantl wrote:

Thanks for jumping on this Amara — I just wanted to point out that we ususally don't revert lldb changes that only break the lldb-xcode bot if they pass on the lldb-cmake bot at the same time. When this happens it usually means that the lldb Xcode project must be updated and it's too much to ask from all open source contributors to get access to a machine running Xcode to do this. Instead one of the Apple LLDB developers usually goes in and updates the Xcode project for them.

• adrian

Hi Pavel,