[PATCH] D138488: [AArch64][clang] implement 2022 General Data-Processing instructions

dmgreen added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:11725 +: BaseTwoOperandRegReg, + Sched<[]> { + let Inst{11} = isMin; stuij wrote: > dmgreen wrote: > > Can we make this WriteI, maybe. I think that would probably

[PATCH] D138488: [AArch64][clang] implement 2022 General Data-Processing instructions

dmgreen accepted this revision. dmgreen added a comment. Thanks. I haven't looked though all the details, but thanks for fixing the scheduling info. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138488/new/ https://reviews.llvm.org/D138488 __

[PATCH] D138579: [AArch64] Assembly support for FEAT_LRCPC3

dmgreen added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:11772 + : I, +Sched<[]> { + bits<5> Rt; One extra nit: Can we add a scheduling description? These sound like they can use WriteAtomic. Repository: rG LLVM

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

dmgreen added a comment. I can only comment on the target features part of the patch - I've been hoping it would add something similar and looks very useful in its own right. Comment at: clang/lib/Basic/Targets/AArch64.cpp:63 + switch (ArchKind) { + case llvm::AArch64::ArchK

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

dmgreen added inline comments. Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:115 +AARCH64_ARCH_EXT_NAME("rdm", AArch64::AEK_RDM, "+rdm", "-rdm", \ +RDM, "+rdm,+fp-armv8,+neon,+jsconv,+complxnum",

[PATCH] D76491: [ARM,MVE] Add ACLE intrinsics for the vaddv/vaddlv family.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Sounds great, from what I can see. The predicated lowering looks useful when/if we try and get predicated vecreduce's working. We'll have to do the same for VMLA at some point. LGTM Reposi

[PATCH] D74966: [PATCH] [ARM] Add Cortex-M55 Support for clang and llvm

dmgreen added inline comments. Comment at: llvm/lib/Target/ARM/ARM.td:625 - //===--===// Seems unrelated ;) Comment at: llvm/test/CodeGen/ARM/build-attributes.ll:1376 ;

[PATCH] D75252: [ARM,MVE] Add ACLE intrinsics for VQMOV[U]N family.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. I had some code somewhere that was trying to produce VQMOVNbh from codegen, treating them as a saturating truncate. It would only work in certain situations thought, relying on demand bits t

[PATCH] D75255: [ARM,MVE] Add ACLE intrinsics for VCVT[ANPM] family.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Sound good to me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75255/new/ https://reviews.llvm.org/D75255 _

[PATCH] D69788: [ARM MVE] Remove accidental 64-bit vst2/vld2 intrinsics.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM. I was trying to think of a way to test this, but couldn't come up with anything sensible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D69790: [ARM,MVE] Integer-type nitpicks in MVE intrinsics.

dmgreen added inline comments. Comment at: clang/utils/TableGen/MveEmitter.cpp:559 + // Emit code to generate this result as a Value *. + std::string asValue() override { +if (AddressType) Can you explain this, is it related to something else? Repository:

[PATCH] D69789: [clang,MveEmitter] Fix sign/zero extension in range limits.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69789/new/ https://reviews.llvm.org/D69789 _

[PATCH] D69790: [ARM,MVE] Integer-type nitpicks in MVE intrinsics.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM then. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69790/new/ https://reviews.llvm.org/D69790 ___

[PATCH] D69791: [ARM,MVE] Add intrinsics for gather/scatter load/stores.

dmgreen added inline comments. Comment at: clang/include/clang/Basic/arm_mve_defs.td:175 +// CopyKind expects t and u to be scalars. It returns a scalar +// whose kind (signed, unsigned or float) matches that of k, and whose Should the t and u be s and k? ===

[PATCH] D69791: [ARM,MVE] Add intrinsics for gather/scatter load/stores.

dmgreen added a comment. Looks good as far as I can see. Comment at: clang/include/clang/Basic/arm_mve.td:78 +def _gather_base: Intrinsic< + Vector, (args VecOf>:$addr, imm_mem7bit:$offset), + (IRInt<"vldr_gather_base", [Vector, VecOf>]> $addr, $offset)>; ---

[PATCH] D69791: [ARM,MVE] Add intrinsics for gather/scatter load/stores.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Nice. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69791/new/ https://reviews.llvm.org/D69791 __

[PATCH] D70088: [ARM,MVE] Add intrinsics for contiguous load/stores.

dmgreen added a comment. Very nice Just to check, we don't have to care about big endian here? Is just works OK because the rest of llvm handles it OK? (I'm not sure if a vld1 is different to a vldr in big endian, for example). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D70133: [ARM, MVE] Add intrinsics for 'administrative' vector operations.

dmgreen added a comment. Smaller patches are easier to review, if for example this could have the vgetq_lane/vsetq_lane split out. Comment at: clang/include/clang/Basic/arm_mve.td:384 + let params = !foldl([], T.All, tlist, srctype, !listconcat(tlist, + !if(!eq(!cast(desttyp

[PATCH] D70088: [ARM,MVE] Add intrinsics for contiguous load/stores.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. OK. vldr and vld1 working differently for Neon under BE, if I'm remembering correctly. LGTM then. Comment at: clang/utils/TableGen/MveEmitter.cpp:475 // Mostly, when a

[PATCH] D70188: [ARM,MVE] Add intrinsics for vector get/set lane.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70188/new/ https://reviews.llvm.org/D70188

[PATCH] D70133: [ARM, MVE] Add intrinsics for 'administrative' vector operations.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks. That, er, makes it slightly simpler to review. LGTM Comment at: clang/test/CodeGen/arm-mve-intrinsics/admin.c:1671 +// +float16x8_t test_vuninitializedq_polymorphi

[PATCH] D70297: [ARM,MVE] Add intrinsics for vector comparisons.

dmgreen added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:6798 +static llvm::Value *ARMMVEDupVector(CGBuilderTy &Builder, llvm::Value *V) { + // Helper function to duplicate a scalar value V into all lanes of an MVE Is this the same as or simi

[PATCH] D70297: [ARM,MVE] Add intrinsics for vector comparisons.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70297/new/ https://reviews.llvm.org/D70297 _

[PATCH] D70319: [ARM,MVE] Add intrinsics for scalar shifts.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. > That makes even LSLL and ASRL different enough from standard LLVM IR shift > semantics that I couldn't see any better alternative than to simply model the > whole family as a set of MVE-sp

[PATCH] D70485: [ARM,MVE] Add intrinsics to deal with predicates.

dmgreen added inline comments. Comment at: llvm/include/llvm/IR/IntrinsicsARM.td:780 def int_arm_vctp8 : Intrinsic<[llvm_v16i1_ty], [llvm_i32_ty], [IntrNoMem]>; def int_arm_vctp16 : Intrinsic<[llvm_v8i1_ty], [llvm_i32_ty], [IntrNoMem]>; Should we rename int_

[PATCH] D70485: [ARM,MVE] Add intrinsics to deal with predicates.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks, LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70485/new/ https://reviews.llvm.org/D70485 _

[PATCH] D70485: [ARM,MVE] Add intrinsics to deal with predicates.

dmgreen accepted this revision. dmgreen added a comment. Thanks. I'm still happy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70485/new/ https://reviews.llvm.org/D70485 ___ cfe-commits mailing list c

[PATCH] D70546: [ARM][MVE][Intrinsics] Add MVE VMUL intrinsics.

dmgreen added inline comments. Comment at: clang/test/CodeGen/arm-mve-intrinsics/vmulq.c:12 +// +uint32x4_t test_vmulq_u32(uint32x4_t a, uint32x4_t b) +{ I think its worth adding some extra tests types. The way I like to think of it is that if someone went and c

[PATCH] D70545: [ARM][MVE][Intrinsics] Add MVE VABD intrinsics.

dmgreen added inline comments. Comment at: clang/include/clang/Basic/arm_mve.td:33 let params = T.Int in { +def vabdq: Intrinsic $a, $b)>; def vaddq: Intrinsic; Can this and vadbqf below be combined into one using T.Usual? I believe the differences only usuall

[PATCH] D69350: [ARM] Replace arm_neon_vqadds with sadd_sat

This revision was automatically updated to reflect the committed changes. Closed by commit rG9f15fcc2718f: [ARM] Replace arm_neon_vqadds with sadd_sat (authored by dmgreen). Herald added a project: clang. Herald added a subscriber: cfe-commits. Changed prior to commit: https://reviews.llvm.org/

[PATCH] D70829: [ARM][MVE][Intrinsics] Add VMINQ/VMAXQ/VMINNMQ/VMAXNMQ intrinsics.

dmgreen added inline comments. Comment at: llvm/test/CodeGen/Thumb2/mve-intrinsics/vmaxnmq.ll:7 +; CHECK: @ %bb.0: @ %entry +; CHECK-NEXT:vcmp.f16 ge, q0, q1 +; CHECK-NEXT:vpsel q0, q0, q1 You may want to use llvm.minnum directly (providing the sema

[PATCH] D70829: [ARM][MVE][Intrinsics] Add VMINQ/VMAXQ/VMINNMQ/VMAXNMQ intrinsics.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks. I'm happy if simon does not have any extra comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70829/new/ https://reviews.llvm.org

[PATCH] D70948: [ARM][MVE][Intrinsics] Add VMULH/VRMULH intrinsics.

dmgreen added a comment. Looks good to me, with a couple of minor nitpicks (that I'm not sure matter or not). Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3626 -class MVE_VxMULH size, +class MVE_VxMULH size, bit U, bit round, list pattern=[]>

[PATCH] D70948: [ARM][MVE][Intrinsics] Add VMULH/VRMULH intrinsics.

dmgreen added inline comments. Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3645 +Intrinsic pred_int, bit round> { + def "" : MVE_VxMULH; + Should this be `VTI.Suffix, VTI.Unsigned, VTI.Size, round` now? Repository: rG LLVM Github M

[PATCH] D70948: [ARM][MVE][Intrinsics] Add VMULH/VRMULH intrinsics.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM. Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70948/new/ https://reviews.llvm.org/D70948 _

[PATCH] D71190: [ARM][MVE] Add complex vector intrinsics

dmgreen added inline comments. Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:2537 +/// Convert an SDValue to a boolean value. SDVal must be a compile-time constant +static bool SDValueToConstBool(SDValue SDVal) { + ConstantSDNode *SDValConstant = dyn_cast(SDVal);

[PATCH] D71198: [ARM][MVE][Intrinsics] Add VQADDQ, VHADDQ, VRHADDQ, VQSUBQ, VHSUBQ, VQDMULHQ, VQRDMULHQ intrinsics.

dmgreen added a comment. Looks nice and clean. I just think we can come up with a better name than adding a _ on the end of it! Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1591 -class MVE_VQxDMULH size, bit rounding, +class MVE_VQxDMULH_ size, bit rounding,

[PATCH] D71198: [ARM][MVE][Intrinsics] Add VQADDQ, VHADDQ, VRHADDQ, VQSUBQ, VHSUBQ, VQDMULHQ, VQRDMULHQ intrinsics.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks. Looks good to me, if no one else has any comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71198/new/ https://reviews.llvm.org/D

[PATCH] D71245: [ARM][MVE] Refactor complex vector intrinsics [NFCI]

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Nice one. Thanks. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71245/new/ https://reviews.llvm.org/D71245 __

[PATCH] D71065: [ARM][MVE] Add intrinsics for immediate shifts.

dmgreen accepted this revision. dmgreen added a comment. One of the advantages to smaller patches I guess :) It's probably difficult to tell if this will cause problems again without trying it and see if any of the buildbots complain. Lets give it a try and see. Just keep an eye on them, we ann

[PATCH] D75998: [ARM,MVE] Add intrinsics and isel for MVE fused multiply-add.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM. Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75998/new/ https://reviews.llvm.org/D75998 _

[PATCH] D76122: [ARM,MVE] Add intrinsics and isel for MVE integer VMLA.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:14289-14292 case Intrinsic::arm_neon_vqrshifts: case Intrinsic::arm_neon_vqrshiftu: // No immediate vers

[PATCH] D73786: [ARM,MVE] Fix vreinterpretq in big-endian mode.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Big endian making things fun again. VECTOR_REG_CAST looks useful for lowering too. LGTM Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3958 // Occasionally we need to c

[PATCH] D73906: [ARM] Make ARM::ArchExtKind use 64-bit underlying type, NFCI

dmgreen added a comment. I added some `This?`'s around, which may or may not be needed too. Comment at: llvm/include/llvm/Support/ARMTargetParser.h:196 unsigned DefaultFPU; unsigned ArchBaseExtensions; T ID; This? Comment at: llvm/li

[PATCH] D73906: [ARM] Make ARM::ArchExtKind use 64-bit underlying type, NFCI

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73906/new/ https://reviews.llvm.org/D73906 ___ cfe-commits mailing list cf

[PATCH] D74334: [ARM,MVE] Add the vrev16q, vrev32q, vrev64q family.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Sounds good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74334/new/ https://reviews.llvm.org/D74334 ___

[PATCH] D74336: [ARM,MVE] Add the vmovlbq,vmovltq intrinsic family.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. I think these odd shuffle then extend patterns might come up in codegen too, so this looks like a nice change. LGTM, if the test is just a test case problem. Comment at:

[PATCH] D74620: [ARM,MVE] Add vector-scalar intrinsics

dmgreen added a comment. I like how this uses a splat for all the register arguments. That sounds like a good idea. The one's that worry me are the floating point instructions. Last time we tried those it was actually causing performance regressions because of extra sp->gpr mov's left in the l

[PATCH] D74336: [ARM,MVE] Add the vmovlbq,vmovltq intrinsic family.

dmgreen accepted this revision. dmgreen added a comment. Nice one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74336/new/ https://reviews.llvm.org/D74336 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D74337: [ARM,MVE] Add the vmovnbq,vmovntq intrinsic family.

dmgreen added inline comments. Comment at: clang/test/CodeGen/arm-mve-intrinsics/vmovn.c:10 +// CHECK-NEXT:[[TMP0:%.*]] = shufflevector <16 x i8> [[A:%.*]], <16 x i8> undef, <16 x i32> +// CHECK-NEXT:[[TMP1:%.*]] = bitcast <16 x i8> [[TMP0]] to <8 x i16> +// CHECK-NEXT:

[PATCH] D74337: [ARM,MVE] Add the vmovnbq,vmovntq intrinsic family.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74337/new/ https://reviews.llvm.org/D74337

[PATCH] D74620: [ARM,MVE] Add vector-scalar intrinsics

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM. Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:4566 + 0b0, VTI.Unsigned>; + defvar unpred_op = !if(VTI.Unsigned, unpred_op_u, unpred_op_s);

[PATCH] D72934: [ARM,MVE] Support immediate vbicq,vorrq,vmvnq intrinsics.

dmgreen added a comment. What is the reason that this can't be lowered in tablegen, in the same way as the VMOVimm's are? For vbic vs vmovlb, the vmovlb does include a free register move, so may under some circumstances be slightly better. Like you say, it's mostly benign, but may be worth upd

[PATCH] D72934: [ARM,MVE] Support immediate vbicq,vorrq,vmvnq intrinsics.

dmgreen added a comment. In D72934#1829387 , @simon_tatham wrote: > In D72934#1829331 , @dmgreen wrote: > > > What is the reason that this can't be lowered in tablegen, in the same way > > as the VMOVimm's are? >

[PATCH] D72934: [ARM,MVE] Support immediate vbicq,vorrq,vmvnq intrinsics.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Looks good, from what I can tell. I especially like the selects. We know that we have to do more work there, but adding this for more instructions would go a long way towards creating more

[PATCH] D73356: [ARM,MVE] Add intrinsics for vdupq.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73356/new/ https://reviews.llvm.org/D73356

[PATCH] D73357: [ARM,MVE] Add intrinsics for v[id]dupq and v[id]wdupq.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Looks good, from what I can tell. Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:2743 + uint16_t Opcode; + switch (VT.getVectorElementType().getSizeInBits()) { + ca

[PATCH] D142135: [AArch64] Basic target("+crypto") handling

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG43aa293aeaf0: [AArch64] Basic target(+crypto) handling (authored by dmgreen). Herald added a project: clang. Herald added a subscriber: cfe-commits.

[PATCH] D147497: [AArch64] Use fneg instead of fsub -0.0, X Cin IR expansion of __builtin_neon_vfmsh_f16.

dmgreen accepted this revision. dmgreen added a comment. Sounds OK to me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147497/new/ https://reviews.llvm.org/D147497 ___ cfe-commits mailing list cfe-commit

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

dmgreen added a comment. Hello - I had to revert this because of some large regressions we got from routines in CMSIS-DSP. The llvm/test/Transforms/PhaseOrdering/ARM/arm_mult_q15.ll test shows the problem - that's why that test exists to ensure that any pipeline changes don't negatively affect

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

dmgreen added a comment. > It’s not clear from the original commit message why the test is related to > inlining order? It seems entirely testing vectorization cost model which > should be insensitive to these kind of changes, right? It's a phase ordering test - it's testing the entire pipeline

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

dmgreen added a comment. > I’ll take a look, but this indicates to me that there’s something missing > from the vectoriser or later passes rather than a problem with the inliners > behaviour. Sure. I'm not saying that this patch is wrong. I'm just saying that unfortunately it leads to some pre

[PATCH] D127910: [Clang][AArch64][SME] Add vector load/store (ld1/st1) intrinsics

dmgreen added inline comments. Comment at: clang/utils/TableGen/SveEmitter.cpp:1477 + + OS << "#if !defined(__ARM_FEATURE_SME)\n"; + OS << "#error \"SME support not enabled\"\n"; bryanpkc wrote: > dmgreen wrote: > > We have been changing how the existing SVE an

[PATCH] D109239: Add support for floating-option `-ffp-eval-method` and for new `pragma clang fp eval-method`

dmgreen added a comment. Hi. I created this issue about the use of Ofast and -1: https://github.com/llvm/llvm-project/issues/60781 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109239/new/ https://reviews.llvm.org/D109239 ___ cfe-commits mail

[PATCH] D147266: [AArch64] Sink operands to allow for bitselect instructions

dmgreen added a comment. Thanks for working on this. I noticed there was another instance of vbsl being reported recently in https://github.com/llvm/llvm-project/issues/62642. Hopefully it can be addresses via extra optimizations too. Can you add a testcase for the issues in https://github.com

[PATCH] D147266: [AArch64] Sink operands to allow for bitselect instructions

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks. LGTM with a few extra suggestions. Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14357 + // Non-mask operands of both Ands should also be in

[PATCH] D147266: [AArch64] Sink operands to allow for bitselect instructions

dmgreen accepted this revision. dmgreen added a comment. Thanks. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147266/new/ https://reviews.llvm.org/D147266 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D146987: [Assignment Tracking] Enable by default

dmgreen added a comment. Hello, I also noticed this potentially causing problems for scalable vectors: https://godbolt.org/z/qdr8P86aW That probably counts as one of the "edge cases for things we hadn't accounted for". Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D148919: [Clang][Sema] Fix invalid cast when validating SVE types within CheckVariableDeclarationType.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. This LGTM, but I don't remember why it was written this way. (I am not an expert in this area either). It seems to still give correct error messages with/without `__attribute__((target("sve"

[PATCH] D147266: [AArch64] Add IR intrinsics for vbsl* C intrinsics

dmgreen added a comment. My preference would be for fixing the code we have, not introducing new intrinsics. Intrinsics act as black-boxes for the optimizer, and I'm pretty sure I've heard of cases in the past of the compiler optimizing the or/and/xor's to nicer sequences of instructions. It wo

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

dmgreen added subscribers: nikic, spatel, efriedma. dmgreen added a comment. Hello. It sounds like it is really close to being OK. The combine of the shift just seem to make things more difficult. The `icmp ult i1 %cmp4.i, true` is just a not, would it help if it was actually an xor? Or if the

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

dmgreen added a comment. It looks like there is quite a lot more optimization that happens to the function being always-inlined (__SSAT) before this change. Through multiple rounds of instcombine, almost to the end of the pass pipeline. The new version runs a lot less before inlining, only runn

[PATCH] D139525: Add implementation isTargetCanonicalConstantNode for hexagon.

dmgreen added a reviewer: kparzysz. dmgreen added a comment. Can you add the test cases from D137140 ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139525/new/ https://reviews.llvm.org/D139525 ___

[PATCH] D117202: [ARM] Add Cortex-X1C Support for Clang and LLVM

dmgreen added a comment. Thanks for the update. Same(ish) questions for the Arm side. Otherwise this LGTM Comment at: llvm/include/llvm/Support/ARMTargetParser.def:332 +ARM_CPU_NAME("cortex-x1c", ARMV8_2A, FK_CRYPTO_NEON_FP_ARMV8, false, + ARM::AEK_RAS) ARM_CPU_NAM

[PATCH] D117202: [ARM] Add Cortex-X1C Support for Clang and LLVM

dmgreen added a comment. Oh, also is there an ID for this CPU that is worth adding to getHostCPUNameForARM in Host.cpp? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117202/new/ https://reviews.llvm.org/D117202 ___

[PATCH] D117753: [AArch64] Support for memset tagged intrinsic

dmgreen added inline comments. Comment at: clang/lib/Headers/arm_acle.h:736 +#define __arm_mops_memset_tag(tagged_address, value, size) \ + __builtin_arm_mops_memset_tag(tagged_address, value, size) +#endif The arguments are better named __v

[PATCH] D117753: [AArch64] Support for memset tagged intrinsic

dmgreen added inline comments. Comment at: clang/lib/Headers/arm_acle.h:736 +#define __arm_mops_memset_tag(tagged_address, value, size) \ + __builtin_arm_mops_memset_tag(tagged_address, value, size) +#endif dmgreen wrote: > The arguments are

[PATCH] D117202: [ARM] Add Cortex-X1C Support for Clang and LLVM

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117202/new/ https://reviews.llvm.org/D117202 _

[PATCH] D117753: [AArch64] Support for memset tagged intrinsic

dmgreen added inline comments. Comment at: clang/lib/Headers/arm_acle.h:734 +/* Memory Operations Intrinsics */ +#if __ARM_FEATURE_MOPS && __ARM_FEATURE_MEMORY_TAGGING +#define __arm_mops_memset_tag(tagged_address, value, size) \ tyb0807 wrot

[PATCH] D117753: [AArch64] Support for memset tagged intrinsic

dmgreen added inline comments. Comment at: clang/lib/Headers/arm_acle.h:734 +/* Memory Operations Intrinsics */ +#if __ARM_FEATURE_MOPS && __ARM_FEATURE_MEMORY_TAGGING +#define __arm_mops_memset_tag(tagged_address, value, size) \ SjoerdMeijer

[PATCH] D118044: [ARM] Undeprecate complex IT blocks

dmgreen added a reviewer: samparker. dmgreen added inline comments. Comment at: llvm/test/CodeGen/ARM/hoist-and-by-const-from-lshr-in-eqcmp-zero.ll:498 +; THUMB8-NEXT:it pl +; THUMB8-NEXT:lslpl.w r1, r0, r ; THUMB8-NEXT:lsl.w r0, r0, r2 Can you make

[PATCH] D117753: [AArch64] Support for memset tagged intrinsic

dmgreen accepted this revision. dmgreen added a comment. Thanks. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117753/new/ https://reviews.llvm.org/D117753 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D117795: [AArch64] Add some missing strict FP vector lowering

dmgreen added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1484 + // of the vector comparison instructions. + setOperationAction(ISD::STRICT_FSETCCS, VT, Expand); + // FIXME: We could potentially make use of the vector comparison instructions --

[PATCH] D117795: [AArch64] Add some missing strict FP vector lowering

dmgreen added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1484 + // of the vector comparison instructions. + setOperationAction(ISD::STRICT_FSETCCS, VT, Expand); + // FIXME: We could potentially make use of the vector comparison instructions --

[PATCH] D97625: fix check-clang-tools tests that fail due to Windows CRLF line endings

dmgreen added a comment. Hello. I noticed this was applied to the base /test directory in the llvm repo: https://github.com/llvm/llvm-project/tree/main/test Was it intended be in clang-tools-extra: https://github.com/llvm/llvm-project/tree/main/clang-tools-extra/test? I'm not sure it will do mu

[PATCH] D119720: [ARM] Pass for Cortex-A57 and Cortex-A72 Fused AES Erratum

dmgreen added a comment. I have a high level question regarding RDF, as I've not seen it used in many other places, so it may be under-tested on Arm systems at the moment. This currently, for all code, builds an rdf graph, analyze the rdf graph for a fairly rare instructions, then fixes up the

[PATCH] D120875: [Driver] Split up huge aarch64-cpus.c test.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. OK Cool. Lets not punish Florian for improving things. If anyone want to go and split this file sensibly, that sounds good. In the meantime this LGTM. Repository: rG LLVM Github Monorepo

[PATCH] D120876: [Driver] Split up huge arm-cortex-cpus.c test.

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Same as D120875 , if anyone wants to split this up more naturally that would be good, but until then this LGTM. Repository: rG LLVM Github Monorepo CHAN

[PATCH] D121206: [AARCH64] ssbs should be enabled by default for cortex-x1c

dmgreen added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64.td:978 FeatureNEON, FeatureRCPC, FeaturePerfMon, FeatureSPE, FeatureFullFP16, FeatureDotProd]; list X1C = [HasV8_2aOps, FeatureCrypto

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

dmgreen added a comment. Hello. Nice idea. Unfortunately this blocks tail folding, making codesize a bit bigger: https://godbolt.org/z/rncPvbh8d I'm guessing it shouldn't? But the attribute isn't handled somewhere along the way. Any ideas where? Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D101606: [ARM][MVE] vcreateq lane ordering for big endian

dmgreen added a comment. Sounds good to me. Whilst we are here, are any of the other uses of bitcast in arm_mve.td potentially a problem? I took a quick look and because they both converting the inputs and the outputs, I believe they will be OK. (Two wrongs make a right, if you will). =

[PATCH] D101606: [ARM][MVE] vcreateq lane ordering for big endian

dmgreen added inline comments. Comment at: clang/test/CodeGen/arm-mve-intrinsics/admin.c:86 // CHECK-LABEL: @test_vcreateq_s64( // CHECK-NEXT: entry: You have to remove the old checks - the script isn't very good at that. What would probably be even better

[PATCH] D101606: [ARM][MVE] vcreateq lane ordering for big endian

dmgreen accepted this revision. dmgreen added a comment. This revision is now accepted and ready to land. Thanks for the updates. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101606/new/ https://reviews.llvm.org/D101606

[PATCH] D100919: [AArch64] Support customizing stack protector guard

dmgreen added a comment. I don't know a huge amount about stack protectors. It's worth adding -verify-machineinstrs to the tests, to check the code passes the internal checks. Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1917 +.addDef(Reg) +

[PATCH] D102238: [TableGen] [Clang] Clean up arm_mve.td file

dmgreen added a reviewer: simon_tatham. dmgreen added a comment. Testing the inc files sounds good to me. There are also tests in places like clang/test/CodeGen/arm-mve-intrinsics that will test this, which seem to all be passing? Anything that does `#include ` Repository: rG LLVM Github Mon

[PATCH] D100919: [AArch64] Support customizing stack protector guard

dmgreen added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1917 +.addDef(Reg) +.addImm(Options.StackProtectorGuardOffset >> 3); + else nickdesaulniers wrote: > dmgreen wrote: > > What is StackProtectorGuar

[PATCH] D116748: [AArch64][ARM][Clang] PerfMon Extension Added

dmgreen added a comment. Hmm. This appears to be mapping pmuv3p4 to "perfmon". But "perfmon" has been around a long time, not a new feature related to the pmuv3p4 update. It seems on the AArch64 side to control access to PMCCNTR, i.e. access to the base FEAT_PMUv3. On the Arm side this is alte

[PATCH] D117112: [AArch64] Support for Ampere1 core

dmgreen added a comment. You could consider adding some scheduling tests, like those in llvm/test/tools/llvm-mca/AArch64/Cortex/A55-basic-instructions.s. In the past we have tended not to add them for schedules, so they are not necessary, but can be useful for checking the details are as you ex

[PATCH] D117202: [ARM] Add Cortex-X1C Support for Clang and LLVM

dmgreen added inline comments. Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:205 +AARCH64_CPU_NAME("cortex-x1c", ARMV8_2A, FK_CRYPTO_NEON_FP_ARMV8, false, + (AArch64::AEK_RAS)) AARCH64_CPU_NAME("cortex-x2", ARMV9A, FK_NEON_FP_ARMV8, false, ---

[PATCH] D116748: [AArch64][ARM][Clang] PerfMon Extension Added

dmgreen accepted this revision. dmgreen added a comment. LGTM. Thanks for the update. Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:147 AARCH64_ARCH_EXT_NAME("sme-i64", AArch64::AEK_SMEI64, "+sme-i64", "-sme-i64") +AARCH64_ARCH_EXT_NAME("pmuv3",

<    1   2   3   4   >