This is an archive of the discontinued LLVM Phabricator instance.

[FastISel] Reuse register for bitcast that does not change MVT
ClosedPublic

Authored by nikic on Feb 10 2022, 5:21 AM.

Details

Summary

The current FastISel code reuses the register for a bitcast that doesn't change the IR type, but uses a reg-to-reg copy if it changes the MVT. However, we can simply reuse the register in that case as well.

In particular, this avoids unnecessary reg-to-reg copies for pointer bitcasts. This was found while inspecting O0 codegen differences between typed and opaque pointers.

Diff Detail

Event Timeline

nikic created this revision.Feb 10 2022, 5:21 AM
nikic requested review of this revision.Feb 10 2022, 5:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2022, 5:21 AM
RKSimon accepted this revision.Feb 11 2022, 5:12 AM

LGTM

This revision is now accepted and ready to land.Feb 11 2022, 5:12 AM
This revision was landed with ongoing or failed builds.Feb 14 2022, 12:15 AM
This revision was automatically updated to reflect the committed changes.
ab added a subscriber: ab.May 11 2022, 12:44 PM

Hi @nikic, I may have hit an issue with this change: when the noop-bitcast result is already used in other (potentially already selected) blocks, we already assigned a vreg for the value, and it may be too late to change it when selecting this bitcast. Concretely, this repro fails for x86_64 with -verify-machineinstrs, because the second use of %tmp1 uses a never-defined vreg:

define void @repro(i8** %a0, i1 %a1) {
  %tmp0 = load i8*, i8** %a0
  %tmp1 = bitcast i8* %tmp0 to void ()*
  call void %tmp1()
  br i1 %a1, label %bb1, label %bb2

bb1:
  call void %tmp1()
  ret void

bb2:
  ret void
}

My fast-isel is way too rusty to suggest an obvious fix, so can you please take a look? ;) Thanks!

Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 12:44 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
In D119432#3507087, @ab wrote:

Hi @nikic, I may have hit an issue with this change: when the noop-bitcast result is already used in other (potentially already selected) blocks, we already assigned a vreg for the value, and it may be too late to change it when selecting this bitcast. Concretely, this repro fails for x86_64 with -verify-machineinstrs, because the second use of %tmp1 uses a never-defined vreg:

define void @repro(i8** %a0, i1 %a1) {
  %tmp0 = load i8*, i8** %a0
  %tmp1 = bitcast i8* %tmp0 to void ()*
  call void %tmp1()
  br i1 %a1, label %bb1, label %bb2

bb1:
  call void %tmp1()
  ret void

bb2:
  ret void
}

My fast-isel is way too rusty to suggest an obvious fix, so can you please take a look? ;) Thanks!

Thanks for the report! I've put up https://reviews.llvm.org/D125459 to fix this issue.