[PATCH] D133239: [RISCV][MC] Add minimal support for Ztso extension

This revision was automatically updated to reflect the committed changes. Closed by commit rGa4a29438f451: [RISCV][MC] Add minimal support for Ztso extension (authored by reames). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

reames created this revision. reames added reviewers: palmer-dabbelt, sunshaoce, craig.topper, kito-cheng, jrtc27, frasercrmck, asb, luismarques. Herald added subscribers: VincentWu, luke957, StephenFan, vkmr, jdoerfert, evandro, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, Pkm

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

reames added inline comments. Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:709 + +let Predicates = [HasStdExtZawrs] in { +def WRS_NTO : RVInstI<0b000, OPC_SYSTEM, (outs), (ins), "wrs.nto", "">, jrtc27 wrote: > This doesn't really belong here, but a separat

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

reames added a comment. In D133443#3790420 , @asb wrote: > The change in set of instructions without changing the version number is > concerning - do you know anyone involved in that group? It would be good to > feedback the difficulties this can cause

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

reames added a comment. In D133443#3792627 , @asb wrote: > I think the summary of our discussion on this was: > > - The versioning confusion is unfortunate - ideally there would be discussion > elsewhere at RVI on improving the situation (either ELF attr

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

reames updated this revision to Diff 460447. reames added a comment. Add docs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133443/new/ https://reviews.llvm.org/D133443 Files: clang/test/Preprocessor/riscv-target-features.c llvm/docs/RISCVUsage.rst llvm/lib/Support/RISCVISAInfo.

[PATCH] D130311: [RISCV] Enable strict FP in clang as long as Zve* or V are not enabled.

reames added a comment. I'm not fluent on strict FP, so let me summarize my understanding. This is mostly so you can easily correct me if one my assumptions is wrong. - Under strict FP, clang will emit constrained fp intrinsics instead of normal floating point ops. - To my knowledge, clang wil

[PATCH] D130311: [RISCV] Enable strict FP in clang as long as Zve* or V are not enabled.

reames added a comment. In D130311#3691146 , @craig.topper wrote: > In D130311#3691029 , @reames wrote: > >> I'm not fluent on strict FP, so let me summarize my understanding. This is >> mostly so you can easil

[PATCH] D130311: [RISCV] Enable strict FP in clang as long as Zve* or V are not enabled.

reames added inline comments. Comment at: clang/lib/Basic/Targets/RISCV.cpp:286 + // StrictFP support for vectors is incomplete. + if (ISAInfo->hasExtension("zve32x")) +HasStrictFP = false; craig.topper wrote: > reames wrote: > > craig.topper wrote: > > > r

[PATCH] D129824: [RISCV] Set triple based on -march flag which can be deduced in more generic way

reames added a comment. This was very briefly discussed at today's sync up call. We were running short on time, so we didn't get a chance to talk through it, but there did seem to be a consensus that discussion on the interface implications was needed. This should hopefully be on the agenda w

[PATCH] D125557: [APInt] Remove all uses of zextOrSelf, sextOrSelf and truncOrSelf

reames added a comment. Coming into this late, but I'd have preferred to see this separated into at least two pieces. One for each "non-obvious" adjustment, and one final one which just did the replace on the renaming sites. This differs from feedback from other reviewers above, so don't feel

[PATCH] D125893: [RISCV][NFC] Change interface of RVVIntrinsic::getSuffixStr

reames accepted this revision. reames 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/D125893/new/ https://reviews.llvm.org/D125893 ___

[PATCH] D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point)

reames added a comment. In D110745#3035975 , @nikic wrote: > Sorry, but the fact that there is still no way to opt-in to the old behavior > is still a blocker from my side. If we can't use `dereferenceable + nofree` > arguments for that purpose, then we

[PATCH] D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point)

reames added a comment. Herald added a subscriber: jeroen.dobbelaere. @nikic ping on previous question. It's been a month, and this has been LGTMed. Without response, I plan to land this. In D110745#3038848 , @xbolva00 wrote: > This really needs to be

[PATCH] D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point)

reames abandoned this revision. reames added a comment. I'm stopping work on this. This has already exceeded the amount of work which is worthwhile for me, and it seems there is yet more work needed. On @nikic's prompting, I finally went ahead and got the test-suite setup and tested a clang ve

[PATCH] D114963: [funcattrs] Infer writeonly argument attribute

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG740057d185ea: [funcattrs] Infer writeonly argument attribute (authored by reames). Herald added subscribers: cfe-commits, kerbowa, atanasyan, jrtc27,

[PATCH] D115003: [funcattrs] Infer writeonly argument attribute [part 2]

reames created this revision. reames added reviewers: jdoerfert, anna, aeubanks, modimo, sstefan1. Herald added subscribers: jeroen.dobbelaere, ormris, kerbowa, bollu, hiraditya, sbc100, nhaehnle, jvesely, mcrosier. reames requested review of this revision. Herald added subscribers: cfe-commits, a

[PATCH] D115021: [funcatts] Rewrite callsite operand handling in memory access inference

reames created this revision. reames added reviewers: jdoerfert, anna, sstefan1, aeubanks, modimo. Herald added subscribers: ormris, bollu, hiraditya, mcrosier. reames requested review of this revision. Herald added projects: clang, LLVM. Herald added a subscriber: cfe-commits. This is a rewrite o

[PATCH] D115003: [funcattrs] Infer writeonly argument attribute [part 2]

reames planned changes to this revision. reames added a comment. Has the same propagation bug fixed in 7b54de5f , need to rework. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D11

[PATCH] D115021: [funcatts] Rewrite callsite operand handling in memory access inference

reames planned changes to this revision. reames added a comment. Needs reworked over previous patch in stack. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115021/new/ https://reviews.llvm.org/D115021 __

[PATCH] D136106: [clang][RISCV] Set vscale_range attribute based on VLEN

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9a8f3b113d05: [clang][RISCV] Set vscale_range attribute based on VLEN (authored by reames). Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D136106: [clang][RISCV] Set vscale_range attribute based on VLEN

reames added a comment. In D136106#3863202 , @reames wrote: > In D136106#3863147 , @craig.topper > wrote: > >>> In the original review, I'd mentioned that MinVLEN was sometimes zero. >>> That's still true, but a

[PATCH] D116735: [RISCV] Adjust RISCV data layout by using n32:64 in layout string

reames added a comment. Just want to note that I have no strong opinion on this patch. It doesn't seem unreasonable, and I'm comfortable with this being an empirically driven decision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116735/new/ ht

[PATCH] D136570: [RISCV] Add Svnapot extension

reames added a comment. https://llvm.org/docs/RISCVUsage.html#extensions needs to updated after this change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136570/new/ https://reviews.llvm.org/D136570 __

[PATCH] D136571: [RISCV] add svinval extension

reames added a comment. Please update https://llvm.org/docs/RISCVUsage.html#extensions It seems like we'd previously accept these instruction without the extension being explicitly named? If so, this is probably worth a release note to document the change in user behavior. Repository: rG L

[PATCH] D136511: [RISCV][clang] Support RISC-V vectors in UninitializedValues.

reames accepted this revision. reames 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/D136511/new/ https://reviews.llvm.org/D136511 ___

[PATCH] D136571: [RISCV] add svinval extension

reames added inline comments. Comment at: llvm/docs/RISCVUsage.rst:56 ``V``Supported + ``Svinval`` Assembly Support ``Zba`` Supported This table is sorted alphabetically, please move above V. Repository: rG LLVM Git

[PATCH] D136571: [RISCV] add svinval extension

reames accepted this revision. reames 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/D136571/new/ https://reviews.llvm.org/D136571 ___

[PATCH] D136817: [RISCV] Add H extension

reames added inline comments. Comment at: llvm/docs/RISCVUsage.rst:54 ``F``Supported + ``H``Supported ``M``Supported If I'm reading the code right here, we only have assembly support here. Given that, shouldn'

[PATCH] D131708: [RISCV] Change how mtune aliases are implemented.

reames accepted this revision. reames added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/docs/ReleaseNotes.rst:197 + +- ``sifive-7-rv32`` and ``sifive-7-rv64`` are no longer supported for `-mcpu`. + Can you given any guidan

[PATCH] D132197: [RISCV] Use Triple::isRISCV/isRISCV32/isRISCV64 helps in some places. NFC

reames accepted this revision. reames added a comment. This revision is now accepted and ready to land. LGTM w/ optional comment. Comment at: clang/lib/Driver/ToolChains/Linux.cpp:741 getTriple().getArch() == llvm::Triple::thumbeb; - const bool IsRISC

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

reames added a comment. In D133443#3795603 , @asb wrote: > Everything that's in this patch looks good to me - it's just missing some > simple round-trip tests in the style of rv32zicboz-valid.s (and perhaps an > -invalid.s that shows a sensible error me

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

reames updated this revision to Diff 461345. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133443/new/ https://reviews.llvm.org/D133443 Files: clang/test/Preprocessor/riscv-target-features.c llvm/docs/RISCVUsage.rst llvm/lib/Support/RISCVISAInfo.cpp llvm/lib/Target/RISCV/RISCV.td

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGeda2af575fdf: [RISCV][MC] Add support for experimental Zawrs extension (authored by reames). Changed prior to commit: h

[PATCH] D133443: [RISCV][MC] Add support for experimental Zawrs extension

reames added a comment. In D133443#3801899 , @asb wrote: > It looks like they're still missing in this updated version of the patch? I have no idea what's going wrong here. I had been very careful to make sure the patch contained the new test file, but

[PATCH] D133834: [RISCV] Remove support for the unratified Zbt extension.

reames accepted this revision. reames added a comment. LGTM as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133834/new/ https://reviews.llvm.org/D133834 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D133863: [RISCV] Add MC support of RISCV zcmt Extension

reames added a comment. Please add to the review description a link to the appropriate specification. Please update docs/RISCVUsage.rst to add Zcmt, and link to the specification. It's impossible to review e.g. encoding without knowing what you're implementing. Comment at:

[PATCH] D135894: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension

reames created this revision. reames added reviewers: asb, frasercrmck, craig.topper. Herald added subscribers: sunshaoce, VincentWu, ctetreau, StephenFan, vkmr, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01,

[PATCH] D135894: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension

reames added a comment. In D135894#3856229 , @craig.topper wrote: > There's also this patch from @frasercrmck https://reviews.llvm.org/D107290 So there is; had not seen that. Looking at it, that patch seems to roll several items together. I'd strongl

[PATCH] D107290: [RISCV] Add support for the vscale_range attribute

reames added a comment. Herald added subscribers: sunshaoce, StephenFan, arichardson. Herald added a project: All. Not knowing about this patch, I posted D135894 which addresses a small sub-set of this. If that goes in, I plan to iterative split off some other

[PATCH] D135894: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension

reames updated this revision to Diff 468239. reames added a comment. fix typo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135894/new/ https://reviews.llvm.org/D135894 Files: clang/lib/Basic/Targets/RISCV.cpp clang/lib/Basic/Targets/RISCV.h clang/test/CodeGen/riscv-vector-bits-v

[PATCH] D135894: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension

This revision was automatically updated to reflect the committed changes. Closed by commit rG4467c781d7bb: [clang][RISCV] Set vscale_range attribute based on presence of "v" extension (authored by reames). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D136106: [clang][RISCV] Set vscale_range attribute based on VLEN

reames created this revision. reames added a reviewer: craig.topper. Herald added subscribers: sunshaoce, VincentWu, StephenFan, vkmr, frasercrmck, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones,

[PATCH] D136106: [clang][RISCV] Set vscale_range attribute based on VLEN

reames added a comment. In D136106#3863147 , @craig.topper wrote: >> In the original review, I'd mentioned that MinVLEN was sometimes zero. >> That's still true, but apparently only happens if you specify multiple >> extensions in the -target_feature s

[PATCH] D136106: [clang][RISCV] Set vscale_range attribute based on VLEN

reames updated this revision to Diff 468344. reames edited the summary of this revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136106/new/ https://reviews.llvm.org/D136106 Files: clang/lib/Basic/Targets/RISCV.cpp clang/test/CodeGen/riscv-vector-bits-vscale-range.c llvm/incl

[PATCH] D136817: [RISCV] Add H extension

reames accepted this revision. reames 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/D136817/new/ https://reviews.llvm.org/D136817 ___

[PATCH] D137266: [RISCV] Move RVVBitsPerBlock to TargetParser.h so we can use it in clang. NFC

reames accepted this revision. reames 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/D137266/new/ https://reviews.llvm.org/D137266 ___

[PATCH] D137280: [RISCV] Prevent autovectorization using vscale with Zvl32b.

reames accepted this revision. reames added a comment. This revision is now accepted and ready to land. LGTM I'm fine with this, but I thought we didn't support zve32f during compilation at all right now? Is this the only issue which needs fixed. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames created this revision. reames added reviewers: craig.topper, asb, frasercrmck, kito-cheng, jrtc27. Herald added subscribers: sunshaoce, VincentWu, StephenFan, vkmr, jdoerfert, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, Mar

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames added inline comments. Comment at: clang/test/Preprocessor/riscv-target-features.c:437 + +// RUN: %clang -target riscv64 -march=rv64ixventanacondops -x c -E -dM %s \ +// RUN: -o - | FileCheck --check-prefix=CHECK-XVENTANACONDOPS-EXT %s Naming wise, is xven

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames updated this revision to Diff 473001. reames added a comment. Fix silly typo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137350/new/ https://reviews.llvm.org/D137350 Files: clang/test/Preprocessor/riscv-target-features.c llvm/docs/RISCVUsage.rst llvm/lib/Support/RISCVISA

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames added a comment. In D137350#3906126 , @craig.topper wrote: > Do these need their own DecoderNameSpace? What is a decoder namespace? Some quick grepping and googling isn't very informative. Comment at: llvm/lib/Target/RISCV/R

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames updated this revision to Diff 473022. reames added a comment. Address review comments from @craig.topper CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137350/new/ https://reviews.llvm.org/D137350 Files: clang/test/Preprocessor/riscv-target-features.c llvm/docs/RISCVUsage.rst

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames updated this revision to Diff 473272. reames added a comment. Add a decoder namespace. I did one per vendor - i.e. used a Ventana table rather than a VentanaCondOps one. In theory, we could have a vendor who ships incompatible extensions (in two different pieces of hardware), but if we

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames updated this revision to Diff 473285. reames added a comment. Rebase CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137350/new/ https://reviews.llvm.org/D137350 Files: clang/test/Preprocessor/riscv-target-features.c llvm/docs/RISCVUsage.rst llvm/lib/Support/RISCVISAInfo.cpp

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames updated this revision to Diff 474620. reames added a comment. Address Kito's review comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137350/new/ https://reviews.llvm.org/D137350 Files: clang/test/Preprocessor/riscv-target-features.c llvm/docs/RISCVUsage.rst llvm/lib/

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

reames updated this revision to Diff 474782. reames added a comment. Address comments from @craig.topper CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137350/new/ https://reviews.llvm.org/D137350 Files: clang/test/Preprocessor/riscv-target-features.c llvm/docs/RISCVUsage.rst llvm

[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG780c53984449: [RISCV] Implement assembler support for XVentanaCondOps (authored by reames). Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D142144: [RISCV][Driver] Add -mrvv-vector-bits= option similar to -msve-vector-bits=

reames added a comment. Generally supportive of having such an option, but going to defer to others on the review. I don't work enough on clang to have an opinion on code here. Comment at: clang/docs/ReleaseNotes.rst:844 take architecture extensions from ``-march`` if both

[PATCH] D141672: [RISCV] Support vector crypto extension ISA string and assembly

reames added a comment. This was brought up in today's RISCV sync call; I want to summarize the major points of discussion. This patch is now behind the current most recent revision on the vector-crypto spec. (https://github.com/riscv/riscv-crypto/releases/download/v20230125/riscv-crypto-spec

[PATCH] D126461: [RISCV] Extract and store new vl of vleff/vlsegff iff destination isn't null

reames requested changes to this revision. reames added a comment. This revision now requires changes to proceed. Despite the comments above, the purpose of this patch remains unclear. Per the draft spec, the relevant wording is: "These instructions execute as a regular load except that they will

[PATCH] D126461: [RISCV] Extract and store new vl of vleff/vlsegff iff destination isn't null

reames added a comment. In D126461#3597862 , @craig.topper wrote: > `dst` in the patch description is not the pointer being loaded, it's the > pointer of where to store the new_vl. That is only thing being checked for > null in this patch. I agree thi

[PATCH] D126461: [RISCV] Extract and store new vl of vleff/vlsegff iff new_vl output pointer isn't null

reames added a comment. @craig.topper Much clearer, thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126461/new/ https://reviews.llvm.org/D126461 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D126461: [RISCV] Extract and store new vl of vleff/vlsegff iff new_vl output pointer isn't null

reames added a comment. Ok, with the revised description, let me start anew in my response. I don't have any particular problem with this change. I do think it would be good to explicitly update the docs to indicate whether the param can be null or not, but given a) gcc has allowed it, and b)

[PATCH] D157362: [RISCV] Add MC layer support for Zicfilp.

reames added inline comments. Comment at: llvm/docs/RISCVUsage.rst:120 ``Zicboz`` Assembly Support + ``Zicfilp`` Assembly Support ``Zicntr`` (`See Note <#riscv-i2p1-note>`__) This in the wrong place; this is not a ratified extensi

[PATCH] D155081: Specify the developer policy around links to external resources

reames added inline comments. Comment at: llvm/docs/DeveloperPolicy.rst:349 -* If the patch has been reviewed, add a link to its review page, as shown - `here `_. Removing this item seems very off

[PATCH] D155081: Specify the developer policy around links to external resources

reames added inline comments. Comment at: llvm/docs/DeveloperPolicy.rst:349 -* If the patch has been reviewed, add a link to its review page, as shown - `here `_. aaron.ballman wrote: > aaron.ball

[PATCH] D155081: Specify the developer policy around links to external resources

reames added inline comments. Comment at: llvm/docs/DeveloperPolicy.rst:349 -* If the patch has been reviewed, add a link to its review page, as shown - `here `_. aaron.ballman wrote: > reames wro

[PATCH] D149248: [RISCV][MC] MC layer support for the experimental zacas extension

reames accepted this revision. reames added a comment. This revision is now accepted and ready to land. LGTM Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:3338 +unsigned Rs2 = Inst.getOperand(2).getReg(); +if (Rd % 2 == 1) { + SMLoc Loc = Operands[

[PATCH] D151730: [RISCV] Support target attribute for function

reames added a comment. Can you separate the change to RISCVAsmPrinter.cpp into it's own review? This looks useful for any case where we have functions in the same model with different function attributes. The __attribute__((target...) syntax is one way to have that, but there are also others

[PATCH] D151730: [RISCV] Support target attribute for function

reames added a comment. FYI, this change appears to be a partial diff. Locally, I need to add RISCV to supportsMultiVersioning() to get this to work at all. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151730/new/ https://reviews.llvm.org/D15173

[PATCH] D151730: [RISCV] Support target attribute for function

reames added a comment. In D151730#4491786 , @craig.topper wrote: > In D151730#4491773 , @jrtc27 wrote: > >> Isn't multiversioning a separate thing that builds on top of per-function >> target attributes? > > Th

[PATCH] D138807: [RISCV] Support vector crypto extension ISA string and assembly

reames added a comment. Some very high level comments for the moment. Until the spec is frozen, these need to be moved into the experimental namespace. You should also add a mention of them under the experimental list in docs/RISCVUsage. A link to the current spec version - along with an exac

[PATCH] D76140: [InlineFunction] update attributes during inlining

reames accepted this revision. reames added a comment. This revision is now accepted and ready to land. LGTM, but with two specific required follow ups. If you're not comfortable committing to both, please don't land this one. Comment at: llvm/lib/Transforms/Utils/InlineFunct

[PATCH] D76140: [InlineFunction] update attributes during inlining

reames accepted this revision. reames added a comment. This revision is now accepted and ready to land. LGTM again, with minor change. p.s. Sorry for missing the functional issue the first time. All of the test changes should have made the issue obvious, but despite reading the LangRef descrip

[PATCH] D16963: Copy LibASTMatchersReference.html to gen'd docs

reames resigned from this revision. reames added a comment. Herald added subscribers: arphaman, mgorny. Resigning from a stale review (2016). Feel free to re-add if thread ever revived. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D16963/new/ https://reviews.llvm.org/D16963 ___

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. I want to chime in support of jyknight's meta comments - particularly the one about the need to balance execution speed vs code size differently in hot vs cold code. For our use case, we have a very large amount of branch dense known cold paths, and being able to only a

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. We uncovered another functional issue with this patch, or at least, the interaction of this patch and other parts of LLVM. In our support for STATEPOINT, PATCHPOINT, and STACKMAP we use N-byte nop sequences for regions of code which might be patched out. It's important

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. I've been digging through the code for this for the last day or so. This is a new area for me, so it's possible I'm off base, but I have some concerns about the current design. First, there appears to already be support for instruction bundling and alignment in the ass

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. In D70157#1771841 , @jyknight wrote: > In D70157#1771832 , @reames wrote: > > > I've been digging through the code for this for the last day or so. This > > is a new area for me, so it's po

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. Recording something so I don't forget it when we get back to the prefix padding version. The write up on the bundle align mode stuff mentions a concerning memory overhead for the feature. Since the basic implementation techniques are similar, we need to make sure we as

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. I just posted an alternate review (https://reviews.llvm.org/D71238) which attempts to carve out a minimum reviewable piece of complexity. The hope is that we can review that one quickly (as there are fewer interacting concerns), and then rebase this one (possibly splitt

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. In offline discussion, there was an agreement that we needed further coordination to make sure this patch moves forward quickly. For that reason, there will be a call happening today at 4pm Pacific. Interested parties are welcome to attend. Zoom Meeting ID: 507-497-88

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. Noting another issue we found in local testing (with an older version of this patch). This interacts badly with the implicit exception mechanism in LLVM. For that mechanism, we end up generating assembly which looks more or less like this: Ltmp: cmp %rsi, (%rdi) j

[PATCH] D70157: Align branches within 32-Byte boundary

reames added a comment. Here are the minutes from our phone call a few minutes ago. Attendees: Andy Kaylor, Craig Topper, Annita Zhang, Tom Stellard, Chandler Carruth, Fedor Sergeev, Philip Reames, Yuanake Luo Status Summary == Performance data has been posted to llvm-dev. We had

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

reames added a comment. In D70157#1787403 , @annita.zhang wrote: > In D70157#1787160 , @MaskRay wrote: > > > > > > > > > There is a precedant: .pushsection/.popsection (MCStreamer::SectionStack). > > With .push_al

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

reames added a comment. In D70157#1788025 , @jyknight wrote: > > .push_align_branch_boundary [N,] [instruction,]* > > I'd like to raise again the possibility of using a more general region > directive to denote "It is allowable to add prefixes/nops before

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

reames added a comment. Specifically on the revised patch, I remain confused by the need for multiple subtypes. The need for fragments *between* the potentially fused instructions doesn't make sense to me. What I was expecting to see was the following: BoundaryAlign w/target=the branch fragmen

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

reames added a comment. I've updated the draft assembler support in https://reviews.llvm.org/D71315 to match the proposal here. Again, this is very much WIP and mostly just to show proposed syntax/usage. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.or

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

reames added a comment. In D70157#1789159 , @skan wrote: > In D70157#1788445 , @reames wrote: > > > Specifically on the revised patch, I remain confused by the need for > > multiple subtypes. The need for fragment

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

reames accepted this revision. reames added a comment. LGTM. The patch isn't perfect, but this has reached the point where landing and iterating in tree is the fastest way to convergence. To be clear, this LGTM *only* applies to the current patch contents, and the *internal only* flag names t

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG14fc20ca6282: Align branches within 32-Byte boundary (NOP padding) (authored by reames). Repository: rG LLVM Github Mon

[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

reames added a comment. I've gone ahead and landed the patch so that we can iterate in tree. See commit 14fc20ca62821b5f85582bf76a467d412248c248 . I've also landed a couple of follow up patches to address issues which would

[PATCH] D146146: [Clang] Stop demoting ElementCount/TypeSize conversion errors to warnings.

reames accepted this revision. reames added a comment. This revision is now accepted and ready to land. This makes perfect sense to me. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146146/new/ https://reviews.llvm.org/D146146 __

[PATCH] D141672: [RISCV] Support vector crypto extension ISA string and assembly

reames added a comment. FYI, zvkb was discussed at the Architectural Review Committee this week and they've asked for some significant changes and expansions. See the notes shared here: https://lists.riscv.org/g/tech-unprivileged/message/450 I actively do not think we need to wait on landing t

[PATCH] D144914: [Clang][Driver] Add -mcpu=help to clang

reames added a comment. Code and description appear out of sync. (help != list) Personally, I like the help naming a lot better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144914/new/ https://reviews.llvm.org/D144914

[PATCH] D144914: [Clang][Driver] Add -mcpu=help to clang

reames accepted this revision. reames 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/D144914/new/ https://reviews.llvm.org/D144914 ___

[PATCH] D148066: [RISCV] Add Smaia and Ssaia extensions support

reames added a comment. I would be fine landing this as experimental before ratification. I see no real downside to doing that, and would essentially leave it the judgement of the patch author as to whether just waiting until ratification or doing the extra work for staging as experimental is

[PATCH] D149495: [RISCV] Add support for V extension in SiFive7

reames added a comment. Let's go ahead and land this as is, we can rework the stylistic pieces once the linked patches land. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149495/new/ https://reviews.llvm.org/D149495 __

[PATCH] D149495: [RISCV] Add support for V extension in SiFive7

reames added a comment. In D149495#4373796 , @michaelmaitland wrote: > In D149495#4373768 , @reames wrote: > >> Let's go ahead and land this as is, we can rework the stylistic pieces once >> the linked patches l

  1   2   >