[NFC][AMDGPU] Do not flush after printing every instruction

[Pierre-vh]

It's very expensive and doesn't achieve anything.

I one test I did, it saves almost 10s on a 2m23s build, bringing it down to 2m15s using a downstream branch.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Pierre van Houtryve (Pierre-vh)

Changes

It's very expensive and doesn't achieve anything.

I one test I did, it saves almost 10s on a 2m23s build, bringing it down to 2m15s using a downstream branch.

---

Full diff: https://github.com/llvm/llvm-project/pull/95237.diff

1 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp (-1)

diff --git a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
index 883b6c4407fe5..227b7383e16d5 100644
--- a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
+++ b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
@@ -43,7 +43,6 @@ void AMDGPUInstPrinter::printRegName(raw_ostream &OS, MCRegister Reg) const {
 void AMDGPUInstPrinter::printInst(const MCInst *MI, uint64_t Address,
                                   StringRef Annot, const MCSubtargetInfo &STI,
                                   raw_ostream &OS) {
-  OS.flush();
   printInstruction(MI, Address, STI, OS);
   printAnnotation(OS, Annot);
 }

[jayfoad]

Nice! Does it speed up lit testing?

[jayfoad]

Could do the same in R600InstPrinter just for symmetry.

[jayfoad]

Does it speed up lit testing?

Answering my own question, check-llvm-codegen-amdgpu seems to be 1% to 2% faster with this patch in my Release+Asserts build.

[nico]

Looks like this breaks tests: http://45.33.8.238/linux/140380/step_11.txt

Please take a look and revert for now if it takes a while to fix.

[Pierre-vh]

Looks like this breaks tests: http://45.33.8.238/linux/140380/step_11.txt

Please take a look and revert for now if it takes a while to fix.

Investigating

[Pierre-vh]

I reverted, it's some weird interaction with the order in which the output is expected in the test.
I'm leaning towards guarding the flush calls so they only run in debug mode. I'll investigate a bit more in case I notice a better alternative.

[shiltian]

Is it because we have some output/print are via stdout and some via stderr?

[Pierre-vh]

Is it because we have some output/print are via stdout and some via stderr?

Yes, and the test relies on this ordering for its output.

I think the right move in the end is to move all those CHECK: error to check for actual line numbers using :[[@LINE+1]]:{{[0-9]+}}: which is the usual pattern I see for this kind of stuff.

[jayfoad]

I would suggest changing the test to have two RUN lines, one to test stdout and one to test stderr. It should not be relying on -NEXT: checks where one line is stderr and the next is stdout!

Or, only flush stdout just before emitting an error message to stderr???

[jayfoad]

I think the right move in the end is to move all those CHECK: error to check for actual line numbers using :[[@LINE+1]]:{{[0-9]+}}: which is the usual pattern I see for this kind of stuff.

Yes!

[kosarev]

This causes MC/Disassembler/AMDGPU/decode-err.txt to fail in amd-gfx11-true16 downstream. Same reason -- stderr is getting flushed before there's enough to flush to stdout, which doesn't agree with the order of the check lines there.

I think the right move in the end is to move all those CHECK: error to check for actual line numbers using :[[@LINE+1]]:{{[0-9]+}}: which is the usual pattern I see for this kind of stuff.

I don't see how this resolves the problem. Given two streams are getting flushed interleaved at random points, it's not even guaranteed that the combined output contains complete lines. Also not going to work where check lines match both stdout and stderr and thus expect them to come in a particular order.

Or, only flush stdout just before emitting an error message to stderr???

I would prefer that to having extra run lines. I remember introducing a utility, called errcat, for one of LLVM-based projects in the past that collects output from the streams and combines them such that the stderr part always come after the stdout one, which worked well.

[arsenm]

You can redirect stderr to a temp file and then just have another FileCheck run

[Pierre-vh]

I don't see how this resolves the problem. Given two streams are getting flushed interleaved at random points, it's not even guaranteed that the combined output contains complete lines. Also not going to work where check lines match both stdout and stderr and thus expect them to come in a particular order.

It resolves the problem because checking stderr/stdout being interleaved isn't a particularly good idea, IMO. With that pattern you only need to check stderr.

You can redirect stderr to a temp file and then just have another FileCheck run

I think this is the right way to do it, yes.
I also don't see why a test checking decoding failures should be checking decoding success. It seems like the test should be split in half

Though, I'm intrigued by the possibility that stdout/stderr could overlap in the output, like in the terminal. I'd need to see a real example of it happening though before I do any changes in that direction

[kosarev]

You can redirect stderr to a temp file and then just have another FileCheck run

Easy, but probably not ideal from performance and disk wearing perspectives.

I don't see how this resolves the problem. Given two streams are getting flushed interleaved at random points, it's not even guaranteed that the combined output contains complete lines. Also not going to work where check lines match both stdout and stderr and thus expect them to come in a particular order.

It resolves the problem because checking stderr/stdout being interleaved isn't a particularly good idea, IMO.

So it's necessary to update the tests that employ that then.

[Pierre-vh]

Easy, but probably not ideal from performance and disk wearing perspectives.

Flushing too often - just because some tests aren't updated - is very likely way worse for performance (and your disk's lifespan). When I removed this flush call after every instruction, It reduced one build's time by almost 10%. 8 seconds wasted doing thousand of premature writes to disk.

So it's necessary to update the tests that employ that then.

Yes, because it's easy to fix and it's not a good pattern.

[kosarev]

Easy, but probably not ideal from performance and disk wearing perspectives.

Flushing too often - just because some tests aren't updated - is very likely way worse for performance (and your disk's lifespan).

Agree, we still should not flush on every instruction. It's just that there are probably better ways than writing to a file.

[arsenm]

I've always hated having the error and non error tests mixed. It would be way easier to understand split tests

[Pierre-vh]

Easy, but probably not ideal from performance and disk wearing perspectives.

Flushing too often - just because some tests aren't updated - is very likely way worse for performance (and your disk's lifespan).

Agree, we still should not flush on every instruction. It's just that there are probably better ways than writing to a file.

If the test is small and you really don't want to create a file (I don't understand why though), you could just run it twice, and one captures errors while the other captures stdout.

I've always hated having the error and non error tests mixed. It would be way easier to understand split tests

It'd be nice to have a way to have separate check lines for stderr and stdout without an intermediate file