This is an archive of the discontinued LLVM Phabricator instance.

[X86][1/2] Support PREFETCHI instructions
ClosedPublic

Authored by pengfei on Oct 16 2022, 8:59 AM.

Diff Detail

Event Timeline

pengfei created this revision.Oct 16 2022, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2022, 8:59 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
pengfei requested review of this revision.Oct 16 2022, 8:59 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 16 2022, 8:59 AM
pengfei updated this revision to Diff 468080.Oct 16 2022, 9:13 AM

Fix lit fails.

pengfei updated this revision to Diff 468109.Oct 16 2022, 7:07 PM

Fix lit fails.

LuoYuanke added inline comments.Oct 17 2022, 1:08 AM
clang/include/clang/Driver/Options.td
4650

I notice in line 4655 the option name is "prfch", do we need to follow the naming convention?

llvm/lib/Target/X86/X86InstrInfo.td
3007

Could you add comments to explain the what the constant (1, 3, 0) means? I guess it is the same to the arguments that llvm.prefetch defines.

pengfei updated this revision to Diff 468195.Oct 17 2022, 7:20 AM
pengfei added subscribers: uweigand, t.p.northover.
  1. Address review comments;
  2. Optimize prefetchit0/1 to prefetcht0/1 for non-rip address;
  3. Add semacheck for prefetch write to instruction cache;
  4. Fix a bug that set rw = 1 for prefetchit0/1;

I think the affected ARM and SystemZ tests are not valid before. Could @t.p.northover and @uweigand help to have a look?

clang/include/clang/Driver/Options.td
4650

I don't know the reason why we used prfchw before, but we are now using the option the same as feature name so that user can easily guess the right option.

llvm/lib/Target/X86/X86InstrInfo.td
3007

Added short introduction. Full description is in https://llvm.org/docs/LangRef.html#llvm-prefetch-intrinsic

  1. Add semacheck for prefetch write to instruction cache;

I think the affected ARM and SystemZ tests are not valid before. Could @t.p.northover and @uweigand help to have a look?

This seems to be a semantic change. The Language Reference does not spell out that a write prefetch on the instruction cache is prohibited. In fact, I read it to explicitly state that every flavor of the prefetch intrinsic that doesn't match anything supported on the target architecture should simply be a no-op. (I could imagine that on certain architectures, you might even have such a prefetch, e.g. to handle self-modifying code more efficiently.)

  1. Add semacheck for prefetch write to instruction cache;

I think the affected ARM and SystemZ tests are not valid before. Could @t.p.northover and @uweigand help to have a look?

This seems to be a semantic change. The Language Reference does not spell out that a write prefetch on the instruction cache is prohibited. In fact, I read it to explicitly state that every flavor of the prefetch intrinsic that doesn't match anything supported on the target architecture should simply be a no-op. (I could imagine that on certain architectures, you might even have such a prefetch, e.g. to handle self-modifying code more efficiently.)

Sure, it is possible. But at least for now, there's no real target requires it. Checked with grep -rwn 'llvm.prefetch.*i32 0\s*)' llvm/test/CodeGen/.
The reason I do it is it's ambiguity to backend when lower it to a target that has "write data", "read data" and "read instruction" prefetches. It's clear if we just support the semantic of "read instruction" and lower it to "read data" when available.
I think we can modify Language Reference for it.

Sure, it is possible. But at least for now, there's no real target requires it. Checked with grep -rwn 'llvm.prefetch.*i32 0\s*)' llvm/test/CodeGen/.

But that's just within the LLVM sources. My point was that -up to now- this was part of the public LLVM IR spec, so there could by other LLVM users out there creating this IR, which would now suddenly break. Not sure what the rules are for breaking IR changes, but that would presumably need some wider discussion.

There are too many things in this patch. Please up MC layer changes from intrinsic/builtin changes. The RIP optimization probably also be separate.

Sure, it is possible. But at least for now, there's no real target requires it. Checked with grep -rwn 'llvm.prefetch.*i32 0\s*)' llvm/test/CodeGen/.

But that's just within the LLVM sources. My point was that -up to now- this was part of the public LLVM IR spec, so there could by other LLVM users out there creating this IR, which would now suddenly break. Not sure what the rules are for breaking IR changes, but that would presumably need some wider discussion.

Thanks @uweigand, you are right. I have split D136145 and put it in Discourse for broad discussion.

pengfei updated this revision to Diff 468463.Oct 18 2022, 2:49 AM

Rebased on D136145 and split RIP optimization.

pengfei retitled this revision from [X86] Support PREFETCHI instructions to [X86][1/2] Support PREFETCHI instructions.Oct 18 2022, 4:04 AM

Can the intrinsics changes be split from this patch so they don't depend on D136145. There's no reason to block assembler/disassembler support for that.

clang/lib/Headers/prfchiintrin.h
17

It looks old that this indented differently than the "Loads" on the line above.

craig.topper added inline comments.Oct 18 2022, 9:01 PM
clang/lib/Headers/prfchiintrin.h
17

That should have said "It looks ODD that this indented differently..."

pengfei updated this revision to Diff 468806.Oct 19 2022, 1:08 AM

Remove the dependence to D136145.

pengfei added inline comments.
clang/lib/Headers/prfchiintrin.h
17

We use this indention in most existing headers. I guess it's intended to align with parameters' description. We follow this rule in the ISAs.

This revision is now accepted and ready to land.Oct 19 2022, 2:16 PM
Matt added a subscriber: Matt.Oct 19 2022, 5:12 PM
This revision was landed with ongoing or failed builds.Oct 19 2022, 6:14 PM
This revision was automatically updated to reflect the committed changes.