Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
On 23/02/18 11:58, Jan Beulich wrote: On 23.02.18 at 12:40, wrote: >> On 22/02/18 13:39, Jan Beulich wrote: >> On 22.02.18 at 13:39, wrote: On 02/22/2018 12:22 PM, Jan Beulich wrote: On 22.02.18 at 12:41, wrote: >> On 22/02/18 11:33, Jan Beulich wrote: >> On 22.02.18 at 11:51, wrote: as-insn-check mutates the passed-in flags. Rename it to as-insn-add, in >> line with cc-option-add. Update all callers. >>> I'm not convinced - cc-option-add makes relatively clear that >>> something is being added to the options passed to CC. If I >>> take as-insn-add this way, the macro would need to add an >>> insn to the AS invocation. While I agree as-insn-check doesn't >>> make clear that it adds any options, I still find this less >>> misleading than the suggested new name. Let's see what >>> others think. >> I'm open to better name suggestions. > The best I can come up with is, well, as-insn-check, as that > reasonably describes at least part of what the construct does. > as-insn-check-and-add-option, besides being too long, isn't > meaningfully better. We're definitely getting into bikeshed territory here. >>> Indeed, but I think a change in name should be an improvement, >>> not going from one questionable name to another questionable >>> one. >>> I agree with Andy that 'check' doesn't really convey that something changed. Is the check-and-add "add it if it doesn't exist already"? Or add it if some other check passes / fails? >>> It is "check if this piece of assembly assembles and add the >>> provided option to the indicated variable", extended by Roger's >>> patch to "..., and add the other provided option if it doesn't >>> assemble". >> Ok - how do we unblock this? >> >> There appears to be agreement that as-insn-check isn't a great name, and >> my proposed as-insn-add isn't much better. >> >> The base runes of as-insn and cc-option are compatible. They check the >> fragment, and yield one of two options. cc-option-add and as-insn-check >> are built on top of the base runes, and mutate the flags passed in. >> >> as-check-frag-update-option ? > as-insn-option-add? Or just as-option-add, considering Roger's > new use cases which don't check insns? Lets go with as-option-add. I'm happy with that. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
>>> On 23.02.18 at 12:40, wrote: > On 22/02/18 13:39, Jan Beulich wrote: > On 22.02.18 at 13:39, wrote: >>> On 02/22/2018 12:22 PM, Jan Beulich wrote: >>> On 22.02.18 at 12:41, wrote: > On 22/02/18 11:33, Jan Beulich wrote: > On 22.02.18 at 11:51, wrote: >>> as-insn-check mutates the passed-in flags. Rename it to as-insn-add, >>> in > line >>> with cc-option-add. Update all callers. >> I'm not convinced - cc-option-add makes relatively clear that >> something is being added to the options passed to CC. If I >> take as-insn-add this way, the macro would need to add an >> insn to the AS invocation. While I agree as-insn-check doesn't >> make clear that it adds any options, I still find this less >> misleading than the suggested new name. Let's see what >> others think. > I'm open to better name suggestions. The best I can come up with is, well, as-insn-check, as that reasonably describes at least part of what the construct does. as-insn-check-and-add-option, besides being too long, isn't meaningfully better. >>> We're definitely getting into bikeshed territory here. >> Indeed, but I think a change in name should be an improvement, >> not going from one questionable name to another questionable >> one. >> >>> I agree with >>> Andy that 'check' doesn't really convey that something changed. Is the >>> check-and-add "add it if it doesn't exist already"? Or add it if some >>> other check passes / fails? >> It is "check if this piece of assembly assembles and add the >> provided option to the indicated variable", extended by Roger's >> patch to "..., and add the other provided option if it doesn't >> assemble". > > Ok - how do we unblock this? > > There appears to be agreement that as-insn-check isn't a great name, and > my proposed as-insn-add isn't much better. > > The base runes of as-insn and cc-option are compatible. They check the > fragment, and yield one of two options. cc-option-add and as-insn-check > are built on top of the base runes, and mutate the flags passed in. > > as-check-frag-update-option ? as-insn-option-add? Or just as-option-add, considering Roger's new use cases which don't check insns? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
Andrew Cooper writes ("Re: [PATCH] build: Rename as-insn-check to as-insn-add"):
> Ok - how do we unblock this?
>
> There appears to be agreement that as-insn-check isn't a great name, and
> my proposed as-insn-add isn't much better.
>
> The base runes of as-insn and cc-option are compatible. They check the
> fragment, and yield one of two options. cc-option-add and as-insn-check
> are built on top of the base runes, and mutate the flags passed in.
>
> as-check-frag-update-option ?
My preferred bikeshed colour is `as-insn-add'. I agree that simply
`check' is wrong but would be content with almost anything else.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
On Fri, Feb 23, 2018 at 11:40:34AM +, Andrew Cooper wrote: > On 22/02/18 13:39, Jan Beulich wrote: > On 22.02.18 at 13:39, wrote: > >> On 02/22/2018 12:22 PM, Jan Beulich wrote: > >> On 22.02.18 at 12:41, wrote: > On 22/02/18 11:33, Jan Beulich wrote: > On 22.02.18 at 11:51, wrote: > >> as-insn-check mutates the passed-in flags. Rename it to as-insn-add, > >> in line > >> with cc-option-add. Update all callers. > > I'm not convinced - cc-option-add makes relatively clear that > > something is being added to the options passed to CC. If I > > take as-insn-add this way, the macro would need to add an > > insn to the AS invocation. While I agree as-insn-check doesn't > > make clear that it adds any options, I still find this less > > misleading than the suggested new name. Let's see what > > others think. > I'm open to better name suggestions. > >>> The best I can come up with is, well, as-insn-check, as that > >>> reasonably describes at least part of what the construct does. > >>> as-insn-check-and-add-option, besides being too long, isn't > >>> meaningfully better. > >> We're definitely getting into bikeshed territory here. > > Indeed, but I think a change in name should be an improvement, > > not going from one questionable name to another questionable > > one. > > > >> I agree with > >> Andy that 'check' doesn't really convey that something changed. Is the > >> check-and-add "add it if it doesn't exist already"? Or add it if some > >> other check passes / fails? > > It is "check if this piece of assembly assembles and add the > > provided option to the indicated variable", extended by Roger's > > patch to "..., and add the other provided option if it doesn't > > assemble". > > Ok - how do we unblock this? > > There appears to be agreement that as-insn-check isn't a great name, and > my proposed as-insn-add isn't much better. > > The base runes of as-insn and cc-option are compatible. They check the > fragment, and yield one of two options. cc-option-add and as-insn-check > are built on top of the base runes, and mutate the flags passed in. > > as-check-frag-update-option ? That seems overly long, and TBH I think almost everyone not familiar with the code would have to go an look what this macro does. I'm fine with either as-insn-check, as-insn-add or as-option-add, but I would also like to unblock this. I guess if there's no consensus we just leave the current one? Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
On 22/02/18 13:39, Jan Beulich wrote: On 22.02.18 at 13:39, wrote: >> On 02/22/2018 12:22 PM, Jan Beulich wrote: >> On 22.02.18 at 12:41, wrote: On 22/02/18 11:33, Jan Beulich wrote: On 22.02.18 at 11:51, wrote: >> as-insn-check mutates the passed-in flags. Rename it to as-insn-add, in >> line >> with cc-option-add. Update all callers. > I'm not convinced - cc-option-add makes relatively clear that > something is being added to the options passed to CC. If I > take as-insn-add this way, the macro would need to add an > insn to the AS invocation. While I agree as-insn-check doesn't > make clear that it adds any options, I still find this less > misleading than the suggested new name. Let's see what > others think. I'm open to better name suggestions. >>> The best I can come up with is, well, as-insn-check, as that >>> reasonably describes at least part of what the construct does. >>> as-insn-check-and-add-option, besides being too long, isn't >>> meaningfully better. >> We're definitely getting into bikeshed territory here. > Indeed, but I think a change in name should be an improvement, > not going from one questionable name to another questionable > one. > >> I agree with >> Andy that 'check' doesn't really convey that something changed. Is the >> check-and-add "add it if it doesn't exist already"? Or add it if some >> other check passes / fails? > It is "check if this piece of assembly assembles and add the > provided option to the indicated variable", extended by Roger's > patch to "..., and add the other provided option if it doesn't > assemble". Ok - how do we unblock this? There appears to be agreement that as-insn-check isn't a great name, and my proposed as-insn-add isn't much better. The base runes of as-insn and cc-option are compatible. They check the fragment, and yield one of two options. cc-option-add and as-insn-check are built on top of the base runes, and mutate the flags passed in. as-check-frag-update-option ? ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
>>> On 22.02.18 at 13:39, wrote: > On 02/22/2018 12:22 PM, Jan Beulich wrote: > On 22.02.18 at 12:41, wrote: >>> On 22/02/18 11:33, Jan Beulich wrote: >>> On 22.02.18 at 11:51, wrote: > as-insn-check mutates the passed-in flags. Rename it to as-insn-add, in > line > with cc-option-add. Update all callers. I'm not convinced - cc-option-add makes relatively clear that something is being added to the options passed to CC. If I take as-insn-add this way, the macro would need to add an insn to the AS invocation. While I agree as-insn-check doesn't make clear that it adds any options, I still find this less misleading than the suggested new name. Let's see what others think. >>> >>> I'm open to better name suggestions. >> >> The best I can come up with is, well, as-insn-check, as that >> reasonably describes at least part of what the construct does. >> as-insn-check-and-add-option, besides being too long, isn't >> meaningfully better. > > We're definitely getting into bikeshed territory here. Indeed, but I think a change in name should be an improvement, not going from one questionable name to another questionable one. > I agree with > Andy that 'check' doesn't really convey that something changed. Is the > check-and-add "add it if it doesn't exist already"? Or add it if some > other check passes / fails? It is "check if this piece of assembly assembles and add the provided option to the indicated variable", extended by Roger's patch to "..., and add the other provided option if it doesn't assemble". Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
On 02/22/2018 12:22 PM, Jan Beulich wrote: On 22.02.18 at 12:41, wrote: >> On 22/02/18 11:33, Jan Beulich wrote: >> On 22.02.18 at 11:51, wrote: as-insn-check mutates the passed-in flags. Rename it to as-insn-add, in line with cc-option-add. Update all callers. >>> I'm not convinced - cc-option-add makes relatively clear that >>> something is being added to the options passed to CC. If I >>> take as-insn-add this way, the macro would need to add an >>> insn to the AS invocation. While I agree as-insn-check doesn't >>> make clear that it adds any options, I still find this less >>> misleading than the suggested new name. Let's see what >>> others think. >> >> I'm open to better name suggestions. > > The best I can come up with is, well, as-insn-check, as that > reasonably describes at least part of what the construct does. > as-insn-check-and-add-option, besides being too long, isn't > meaningfully better. We're definitely getting into bikeshed territory here. I agree with Andy that 'check' doesn't really convey that something changed. Is the check-and-add "add it if it doesn't exist already"? Or add it if some other check passes / fails? -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
>>> On 22.02.18 at 12:41, wrote: > On 22/02/18 11:33, Jan Beulich wrote: > On 22.02.18 at 11:51, wrote: >>> as-insn-check mutates the passed-in flags. Rename it to as-insn-add, in >>> line >>> with cc-option-add. Update all callers. >> I'm not convinced - cc-option-add makes relatively clear that >> something is being added to the options passed to CC. If I >> take as-insn-add this way, the macro would need to add an >> insn to the AS invocation. While I agree as-insn-check doesn't >> make clear that it adds any options, I still find this less >> misleading than the suggested new name. Let's see what >> others think. > > I'm open to better name suggestions. The best I can come up with is, well, as-insn-check, as that reasonably describes at least part of what the construct does. as-insn-check-and-add-option, besides being too long, isn't meaningfully better. > cc-option-add and as-insn-check > are basically the same; they make a test based on a proposed construct, > and end up mutating FLAGS. > > The reason I noticed is because Rogers patch adds an option-no case to > as-insn-check. Which doesn't make the name any worse. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
On 22/02/18 11:33, Jan Beulich wrote: On 22.02.18 at 11:51, wrote: >> as-insn-check mutates the passed-in flags. Rename it to as-insn-add, in line >> with cc-option-add. Update all callers. > I'm not convinced - cc-option-add makes relatively clear that > something is being added to the options passed to CC. If I > take as-insn-add this way, the macro would need to add an > insn to the AS invocation. While I agree as-insn-check doesn't > make clear that it adds any options, I still find this less > misleading than the suggested new name. Let's see what > others think. I'm open to better name suggestions. cc-option-add and as-insn-check are basically the same; they make a test based on a proposed construct, and end up mutating FLAGS. The reason I noticed is because Rogers patch adds an option-no case to as-insn-check. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
>>> On 22.02.18 at 11:51, wrote: > as-insn-check mutates the passed-in flags. Rename it to as-insn-add, in line > with cc-option-add. Update all callers. I'm not convinced - cc-option-add makes relatively clear that something is being added to the options passed to CC. If I take as-insn-add this way, the macro would need to add an insn to the AS invocation. While I agree as-insn-check doesn't make clear that it adds any options, I still find this less misleading than the suggested new name. Let's see what others think. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] build: Rename as-insn-check to as-insn-add
as-insn-check mutates the passed-in flags. Rename it to as-insn-add, in line
with cc-option-add. Update all callers.
Signed-off-by: Andrew Cooper
---
CC: George Dunlap
CC: Ian Jackson
CC: Jan Beulich
CC: Konrad Rzeszutek Wilk
CC: Stefano Stabellini
CC: Tim Deegan
CC: Wei Liu
CC: Roger Pau Monné
---
Config.mk | 10 +-
xen/arch/x86/Rules.mk | 14 +++---
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/Config.mk b/Config.mk
index c6f0df9..41683c7 100644
--- a/Config.mk
+++ b/Config.mk
@@ -163,11 +163,11 @@ as-insn = $(if $(shell echo 'void _(void) { asm volatile
( $(2) ); }' \
| $(filter-out -M% %.d -include
%/include/xen/config.h,$(1)) \
-c -x c -o /dev/null - 2>&1),$(4),$(3))
-# as-insn-check: Add an option to compilation flags, but only if insn is
-#supported by assembler.
-# Usage: $(call as-insn-check,CFLAGS,CC,"nop",-DHAVE_GAS_NOP)
-as-insn-check = $(eval $(call as-insn-check-closure,$(1),$(2),$(3),$(4)))
-define as-insn-check-closure
+# as-insn-add: Add an option to compilation flags, but only if insn is
+# supported by assembler.
+# Usage: $(call as-insn-add,CFLAGS,CC,"insn",option-yes)
+as-insn-add = $(eval $(call as-insn-add-closure,$(1),$(2),$(3),$(4)))
+define as-insn-add-closure
ifeq ($$(call as-insn,$$($(2)) $$($(1)),$(3),y,n),y)
$(1) += $(4)
endif
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 1dc5c37..4775336 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -14,13 +14,13 @@ CFLAGS += -msoft-float
$(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
$(call cc-option-add,CFLAGS,CC,-Wnested-externs)
-$(call as-insn-check,CFLAGS,CC,"vmcall",-DHAVE_GAS_VMX)
-$(call as-insn-check,CFLAGS,CC,"crc32 %eax$$(comma)%eax",-DHAVE_GAS_SSE4_2)
-$(call as-insn-check,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_GAS_EPT)
-$(call as-insn-check,CFLAGS,CC,"rdrand %eax",-DHAVE_GAS_RDRAND)
-$(call as-insn-check,CFLAGS,CC,"rdfsbase %rax",-DHAVE_GAS_FSGSBASE)
-$(call as-insn-check,CFLAGS,CC,"rdseed %eax",-DHAVE_GAS_RDSEED)
-$(call as-insn-check,CFLAGS,CC,".equ \"x\"$$(comma)1", \
+$(call as-insn-add,CFLAGS,CC,"vmcall",-DHAVE_GAS_VMX)
+$(call as-insn-add,CFLAGS,CC,"crc32 %eax$$(comma)%eax",-DHAVE_GAS_SSE4_2)
+$(call as-insn-add,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_GAS_EPT)
+$(call as-insn-add,CFLAGS,CC,"rdrand %eax",-DHAVE_GAS_RDRAND)
+$(call as-insn-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_GAS_FSGSBASE)
+$(call as-insn-add,CFLAGS,CC,"rdseed %eax",-DHAVE_GAS_RDSEED)
+$(call as-insn-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
-U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM \
'-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
