X86ISelLowering: Promote 'add nsw' to a wider type

Hi Sanjay,

Some time ago you implemented a sext(add_nsw(x, C)) → add(sext(x), C_sext) transformation in X86ISelLowering
https://reviews.llvm.org/D13757
Is there any reason why this transformation is limited to sexts and doesn’t support zexts?

Thanks,
Artur

Hi Artur -

I don’t think there’s any reason to limit the transform to sexts only; that’s just the case that was apparent in https://llvm.org/bugs/show_bug.cgi?id=20134 , so I limited it to that pattern.

It’s probably worth noting that I’m currently fighting through casts of all kinds in IR (InstCombine) rather than the backend:
https://reviews.llvm.org/D22421
https://reviews.llvm.org/D22271
https://reviews.llvm.org/D22477
https://reviews.llvm.org/D20774
https://llvm.org/bugs/show_bug.cgi?id=27925

I’m very interested to see what kinds of patterns you’re seeing and want to optimize. It’s possible that IR transforms could eliminate the need for the backend fixups…or it could make them harder. Are there bug reports for the cases that you are looking at?

I’m looking a a case very similar to the one in the bug you mentioned. The only difference is that we don’t rely on LLVM index i64 canonicalization (done by sign extension) but instead our front end does it using zero extension because it knows that in our case indices can’t be negative. So, the resulting IR is virtually the same except sexts are replaced by zexts.

I haven’t yet filed any bugs for that.

Artur

Hi Sanjay,

Hi Artur -

I don’t think there’s any reason to limit the transform to sexts only; that’s just the case that was apparent in https://llvm.org/bugs/show_bug.cgi?id=20134 , so I limited it to that pattern.

It’s probably worth noting that I’m currently fighting through casts of all kinds in IR (InstCombine) rather than the backend:

What is the current status of this work? Does it make sense to patch the existing code in X86ISelLowering in order to support looking through zext or the generic solution will be available soon?

Artur

As a side thought, this might be something best done in a CodeGenPrepare type position in the pipeline. The DAG is a little bit late because by that point you’ve lost some information (global information) and may not be able to eliminate all zexts/sexts accordingly. But instcombine may be a little bit early because it’ll hurt targets that prefer it the other way around. You could expand addressing mode stuff in CodeGenPrepare using knownbits or SCEV, maybe? We do something vaguely similar out of tree (but with the opposite goal, as our target only supports 32-bit offsets).

—escha

My current status on InstCombine patches: ongoing…no end in sight. If you look at some of my recent commits, you’ll see that I’m trying to get general InstCombine transforms to work with vector types too. This is somehow related to the earlier cast patches that I listed, but I’ve lost track of how many levels below the original problem I’ve fallen. :slight_smile:

But I don’t think we can solve the zext equivalent of the sext transform done with https://reviews.llvm.org/D13757 in InstCombine, so something else is needed to handle that case.

As escha notes and Quentin mentioned in D13757, CGP is the likely best place for this sort of thing, but if there are x86-specific (LEA) changes that you’re trying to get, then extending D13757 to zexts is probably the least effort.