Make jitoption a common option in superpmi.py

[AndyAyersMS]

Also change from --jitoption to -jitoption per convention where
options taking an arg have just one dash.

Fixes #48919.

[AndyAyersMS]

@BruceForstall PTAL
cc @dotnet/jit-contrib

I have to trigger the collection manually, right?

[AndyAyersMS]

My SPMI runs are seeing some sort of hiccup fetching published builds. @BruceForstall @kunalspathak does this ring any bells?

;; Linux x64 build 

2021-03-01T21:01:33.9622193Z Upload '/datadisks/disk1/workspace/_work/1/a/CoreCLRProduct___Linux_x64_checked.tar.gz' to file container: '#/6375004/CoreCLRProduct___Linux_x64_checked'
2021-03-01T21:01:35.2523436Z Associated artifact 20302233 with build 1017551

;; Linux SPMI arm pmi libraries

2021-03-01T21:04:36.0613020Z Downloading artifacts for build: 1017551
2021-03-01T21:04:37.2607497Z ##[error]Artifact CoreCLRProduct__Linux_x64_checked not found for build 1017551. Please ensure you have published artifacts in any previous phases of the current build.

[kunalspathak]

My SPMI runs are seeing some sort of hiccup fetching published builds. @BruceForstall @kunalspathak does this ring any bells?

Taking a look.

[BruceForstall]

I have to trigger the collection manually, right?

yes. looks like you found that.

Although I still see these in some failure logs:

'CoreclrArguments' object has no attribute 'jitoption'

I don't know about the "failure to fetch" errors you point to.

Maybe you can try a collection locally with your fixes (if you haven't already).

[kunalspathak]

There are 2 failures. All the non-x64 jobs fail to download CoreCLRProduct__Linux_x64_checked because it doesn't find it. The changes in #48171 by @agocke introduced PGOType in the artifact name which is blank in case of superpmi pipelines. The superpmi yml file that downloads the artifacts were not updated in that PR and hence it doesn't find the renamed artifact. That was the reason that we didn't get the clean run over the weekend as well.

@agocke - If this a permanent change, I will fix it by adding the extra _ in superpmi pipeline jobs. Let me know.

The other failure is in x64 jobs and happening because of what @BruceForstall pointed out.

'CoreclrArguments' object has no attribute 'jitoption'

[AndyAyersMS]

@kunalspathak feel free to add that change to this PR if you want (or point met at diffs for it).

Both fixes will be needed to get clean runs.

[kunalspathak]

Pushed and trigged a run - https://dev.azure.com/dnceng/internal/_build/results?buildId=1017890&view=results

[BruceForstall]

LGTM as a targeted fix.

However, I would still like you to address the comments over in #48738 (comment), namely:

1. (nit) change -jitoption to --jitoption
2. support passing -jitoption to asm diffs

[AndyAyersMS]

@BruceForstall over in #48738 (comment) you suggested a one dash -jitoption, so that's the change I made here -- are you sure you want me to undo this and put it back how I had it originally?

[AndyAyersMS]

For asm diffs, seems like maybe jitoption should just impact jit1? And maybe jit2option for jit2?

[BruceForstall]

you suggested a one dash -jitoption, so that's the change I made here

oops, I wasn't thinking clearly. Thanks for doing that change.

For asm diffs, seems like maybe jitoption should just impact jit1? And maybe jit2option for jit2?

Depends what user model you think makes sense. You could use your newly added -jitoption and pass it to both jit1 and jit2 (as -jit2option to superpmi), or you could add another -jit2option switch to be used by asmdiffs. That's more cumbersome, and annoying if you end up just duplicating the same variable in both cases. But it is more flexible. Note that you need to use the correct set of extra variables when generating the textual asm/dump if there is a diff, in the create_replay_artifacts function.

fwiw, I would go with the simple model (-jitoption to superpmi.py is used for both jit1 and jit2) until someone complains that isn't flexible enough for their scenario.

[AndyAyersMS]

Ok, let's land this first and I will follow up with some thing for asm diffs.

[sandreenko]

It looks like after this change we have such warning: 'CoreclrArguments' object has no attribute 'jitoption' when run without it:

>python D:\Sergey\git\runtime\src\coreclr\scripts\superpmi.py collect D:\Sergey\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\corerun.exe "D:\Sergey\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\tracing\eventpipe\eventsourceerror\eventsourceerror\eventsourceerror.dll"
Warning: deleting existing log file D:\Sergey\git\runtime\artifacts\spmi\superpmi.log
================ Logging to D:\Sergey\git\runtime\artifacts\spmi\superpmi.log
SuperPMI collect
Collecting using command:
  D:\Sergey\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\corerun.exe D:\Sergey\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\tracing\eventpipe\eventsourceerror\eventsourceerror\eventsourceerror.dll
Merging MC files
Cleaning MCH file
Creating TOC file
Verifying MCH file
'CoreclrArguments' object has no attribute 'jitoption'

>

[kunalspathak]

Ah, I realized this when reviewing, but thought it was handled. I verified on my local machine and I didn't see that warning. But see, if this helps?

-            if self.coreclr_args.jitoption:
-               for o in self.coreclr_args.jitoption:
+            if hasattr(self.coreclr_args, "jitoption") and self.coreclr_args.jitoption:
+                for o in self.coreclr_args.jitoption:

[sandreenko]

Looks like my repo was in a bad state, after reset/rebuild I don't see the warning, sorry for the false alarm.

[BruceForstall]

@AndyAyersMS , others: I wanted to use -jitoption JitStress=1 but I script my invocations of superpmi.py using Windows batch scripts, and they have a nasty habit of eating = signs, so by the time it gets to superpmi.py, it's -jitoption JitStress 1, which doesn't work. Would it be ok to also accept jitoption JitStress#1 and let superpmi.py replace # with = before passing it on to superpmi.exe? Are there any options where we actually want to pass a #? (It could be some other character that CMD doesn't object to that we don't use)

[kunalspathak]

Are you suggesting to replace # with = if the # is present? Meaning, for those who don't have problem, can they continue passing JitStress=1 ?

[BruceForstall]

Yes, add .replace:

if self.coreclr_args.jitoption:           
    for o in self.coreclr_args.jitoption: 
        repro_flags += "-jitoption", o.replace('#','=')      ### here