Replace llvm.memcpy et al's i1 isVolatile with i8 VolFlags #65748

urnathan · 2023-09-08T13:02:34Z

Modify memcpy, memcpy.inline & memmove intrinsics to change the final i1 isVolatile parameter becomes an i8 VolFlags parameter. The former is true if either (or both) dst and src point to volatile storage. The new VolFlags separates dst and src volatilities to separate bits in the argument value. This allows any expansion code to only mark the volatile accesses as volatile. The motivating use is copying a volatile structure to/from non-volatile storage.

The patch series:

* Changes the intrinsics, documentation and AutoUpdater. The APIs are not updated, and the existing APIs convert false/true to 0/3 as required.
* Update MLIR
* Update Flang
* Update Polly (tests only)
* At this point the compiler builds, but tests fail
* Update LLVM & Clang non-UTC test cases
* Update LLVM & Clang UTC test cases
* At this point the conversion is done, but nothing makes use or can generate the new values.
* Update IRBuilder APIs to specify separate Dst & Src Volatilities. The API change is backwards compatible and a deprecation warning is added in a later patch (it's quite noisy until the remaing conversions are done).
* Update Clang code gen to specify separate Dst & Src Volatilities.
* At this point IRBuilder and Clang are generating more precise volatility information, but no target lowering makes use of it.
* Update TargetLowering APIs. New predicates added to MemTransferInst, and as with the above API change, this is source compatible, but will eventually educes deprecation warnings.
* Update llvm transforms and target lowering APIs. New EmitTargetCodeForMem{cpy,move} added to SelectionDagTargetInfo, source compatible by forwarding to the old API.
* Target-specific changes for the new APIs.
* Motivating testcase.
* Convert remaining old-API uses, but mark (some) locations as candidates for further optimization opportunities.
* Deprecate the compatibility APIs, this alerts out-of-tree maintainers.
* Conversion of AArch64's bespoke memcpy/memmove expansion to use the new EmitTargetCodeForMem{cpy,move} API.

The MLIR and Flang conversions took about 4 hours each -- not having previously looked at those code bases. Thus I believe any out-of-tree conversions will be straight forwards too. Just pay attention to the deprecation warnings and pass the correct tuple.

90 % of this diff are mechanical testcase changes (either via sed or UTC). Most of the rest is fairly mechanical, located via deprecation warnings.

Modify memcpy, memcpy.inline & memmove intrinsics to change the final i1 isVolatile parameter to be an i8 VolFlags parameter. The former is true if either (or both) dst and src point to volatile storage. The new VolFlags separates dst and src volatilities to separate bits in the argument value. This allows any expansion code to only mark the volatile accesses as volatile. The motivating use is copying a volatile structure to/from non-volatile storage. We get better code generation when expanding such one-sided memcpys, as the non-volatile side can (often) be held in registers. Other than changing the intrinsics, the APIs are not adjusted in this patch.

This update MLIR for the intrinsics isVolatile->VolFlags change. The MLIR parameter name remains isVolatile.

Fortran has no concept of 'volatile', but this updates Flang's IR to represent memcpy/memmove's new VolFlags argument. We only pass i8:0 (replacing the previous i1:false).

Update the non-UTC llvm testcases for memcpy/memmove intrinsic isVolatile->VolFlags change. Only the check lines are altered -- existing llvmir using the old boolean is left and altered during read in.

Update the non-UTC clang testcases for memcpy/memmove intrinsic isVolatile->VolFlags change. Only the check lines are altered -- existing llvmir using the old boolean is left and altered during read in.

Regenerate llvm UTC tests for the isVolatile->VolFlags change.

Regenerate clang UTC tests for the isVolatile->VolFlags change. Most of these were first generated via sed, and then regenerated via UTC.

Introduce llvm::MemTransferVolatility. This provides separate Dst & Src volatility flags, along with compatibility functions to convert from and to a single bool. Those will be deprecated in a later patch, so uses generate warnings. The memcpy et al builders now take a MemTransferVolality, and encode it into the VolFlags argument value.

Update Clang's generation of memcpy et al intrinsics to specify separate Dst & Src volatilities. Completes Clang's conversion to the new API.

Add separate dst & src volatility accessors to MemTransferInst. Add MemTransferVolatility to selection dag's getMemcpy, getMemmove. Existing uses are source compatible, but that API will be deprecated in a later patch to catch any remaining uses.

Replace single isVolatile with VolFlags. Add a new SelectionDAGTargetInfo::EmitCodeForMem{Cpy,Move} virtual function with the new API. The default implementation of which forwards to the old API.

Update all the Target code generation to use the new APIs. Does not convert the EmitTargetCodeForMem{cpy,move} implementations.

The motivating testcase for memcpy/memmove VolFlags change. Copying a volatile strucure to/from non-volatile storage should not pessimize the latter. With the new flags, we can hoist the non-volatile object into registers and process it directly there.

joker-eph · 2023-09-11T22:45:20Z

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

@@ -214,14 +214,14 @@ class LLVM_MemcpyIntrOpBase<string name> :
    /*requiresAccessGroup=*/1, /*requiresAliasAnalysis=*/1> {
  dag args = (ins Arg<LLVM_AnyPointer,"",[MemWrite]>:$dst,
                  Arg<LLVM_AnyPointer,"",[MemRead]>:$src,
-                  AnySignlessInteger:$len, I1Attr:$isVolatile);


Seems to me that should handle this as we do with the FastMathFlags: that is a enum attribute modeling the bitfields.

thanks, looking ...

RKSimon · 2023-09-13T15:46:15Z

llvm/lib/Target/X86/X86ISelLowering.cpp

@@ -24790,8 +24790,8 @@ static SDValue LowerVACOPY(SDValue Op, const X86Subtarget &Subtarget,
  return DAG.getMemcpy(
      Chain, DL, DstPtr, SrcPtr,
      DAG.getIntPtrConstant(Subtarget.isTarget64BitLP64() ? 24 : 16, DL),
-      Align(Subtarget.isTarget64BitLP64() ? 8 : 4), /*isVolatile*/ false, false,
-      false, MachinePointerInfo(DstSV), MachinePointerInfo(SrcSV));
+      Align(Subtarget.isTarget64BitLP64() ? 8 : 4), /*Vol=*/{false, false},


For understandability, should we use a MemTransferVolatility builder helper instead of embedding implicit values like this?

good idea, something like MemTransferVolatility().setDst(true/false).setSrc(true/false) ? Do you have preference for names?

No preference for names - do we need to handle runtime bool values or could a few basic hardcoded combos be enough MemTransferVolatility::neverVolatile() / MemTransferVolatility::readVolatile() ?

Just pushed a patch that uses MemTransferVolatility default ctor for the non-volatile case, and allows .Dst(bool), .Src(bool) and/or .All(bool) to explicitly specify. One does need the ability to provide runtime values there.

nikic · 2023-09-15T05:43:26Z

Related discourse thread: https://discourse.llvm.org/t/copy-to-from-volatile-structure/72278

Rather than permit {bool, bool}, use MemTransferVolatility().Dst(bool).Src(bool).All(bool). One can only set bits this way, not clear them.

github-actions bot added backend:ARM backend:AArch64 backend:MSP430 backend:RISC-V backend:Sparc backend:X86 clang:codegen llvm:globalisel flang:driver PGO Profile Guided Optimizations mlir flang Flang issues not falling into any other category backend:loongarch llvm:SelectionDAGP labels Sep 8, 2023

cor3ntin added llvm:SelectionDAG SelectionDAGISel as well and removed llvm:SelectionDAGP labels Sep 9, 2023

urnathan added 14 commits September 9, 2023 20:43

[mlir] Updates for memcpy/memmove intrinsic change

bcfed3e

This update MLIR for the intrinsics isVolatile->VolFlags change. The MLIR parameter name remains isVolatile.

[flang] Updates for memcpy/memmove intrinsics change

516e42d

Fortran has no concept of 'volatile', but this updates Flang's IR to represent memcpy/memmove's new VolFlags argument. We only pass i8:0 (replacing the previous i1:false).

[polly] Test fixes for memcpy/memmove intrinsics change

62b8423

[llvm] Update manual memcpy/move intrinsic testcases

e392ca5

Update the non-UTC llvm testcases for memcpy/memmove intrinsic isVolatile->VolFlags change. Only the check lines are altered -- existing llvmir using the old boolean is left and altered during read in.

[clang] Update manual memcpy/move intrinsic testcases

1467ae3

Update the non-UTC clang testcases for memcpy/memmove intrinsic isVolatile->VolFlags change. Only the check lines are altered -- existing llvmir using the old boolean is left and altered during read in.

[llvm] Update UTC tests for memcpy/memmove intrinsic change

51cbac0

Regenerate llvm UTC tests for the isVolatile->VolFlags change.

[clang] Update UTC tests for memcpy/memmove intrinsic change

f150676

Regenerate clang UTC tests for the isVolatile->VolFlags change. Most of these were first generated via sed, and then regenerated via UTC.

[clang] Update CodeGen for memcpy/memmove volatile changes

1f8bbc0

Update Clang's generation of memcpy et al intrinsics to specify separate Dst & Src volatilities. Completes Clang's conversion to the new API.

[llvm] CodeGen & Transform updates for memcpy/memmove volatility

8c23741

Replace single isVolatile with VolFlags. Add a new SelectionDAGTargetInfo::EmitCodeForMem{Cpy,Move} virtual function with the new API. The default implementation of which forwards to the old API.

[llvm] Target-specific changes for memcpy/memove volatility

53d7aa4

Update all the Target code generation to use the new APIs. Does not convert the EmitTargetCodeForMem{cpy,move} implementations.

urnathan requested review from a team as code owners September 11, 2023 18:47

joker-eph reviewed Sep 11, 2023

View reviewed changes

RKSimon reviewed Sep 13, 2023

View reviewed changes

nikic requested review from nikic and removed request for a team September 14, 2023 07:58

Add Builder API to MemTransferVolatility

c27f074

Rather than permit {bool, bool}, use MemTransferVolatility().Dst(bool).Src(bool).All(bool). One can only set bits this way, not clear them.

llvmbot added mlir:llvm mlir:spirv llvm:support llvm:analysis labels Sep 15, 2023

bogner removed the HLSL HLSL Language Support label Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace llvm.memcpy et al's i1 isVolatile with i8 VolFlags #65748

Replace llvm.memcpy et al's i1 isVolatile with i8 VolFlags #65748

urnathan commented Sep 8, 2023 •

edited

joker-eph Sep 11, 2023

urnathan Sep 13, 2023

RKSimon Sep 13, 2023

urnathan Sep 13, 2023

RKSimon Sep 15, 2023

urnathan Sep 15, 2023

nikic commented Sep 15, 2023

Replace llvm.memcpy et al's i1 isVolatile with i8 VolFlags #65748

Are you sure you want to change the base?

Replace llvm.memcpy et al's i1 isVolatile with i8 VolFlags #65748

Conversation

urnathan commented Sep 8, 2023 • edited

joker-eph Sep 11, 2023

Choose a reason for hiding this comment

urnathan Sep 13, 2023

Choose a reason for hiding this comment

RKSimon Sep 13, 2023

Choose a reason for hiding this comment

urnathan Sep 13, 2023

Choose a reason for hiding this comment

RKSimon Sep 15, 2023

Choose a reason for hiding this comment

urnathan Sep 15, 2023

Choose a reason for hiding this comment

nikic commented Sep 15, 2023

urnathan commented Sep 8, 2023 •

edited