[llvm-dev] PSA+discussion: guessInstructionProperties=0 is now usable

[Alex Bradbury via llvm-dev]

CodeGenTarget::guessInstructionProperties was introduced way back in
r162460[1], intended as a migration aid that can be set to 0 when no
backends require their properties to be inferred. The comment added in
that patch gives as clear an explanation as I could:

 // The instruction properties mayLoad, mayStore, and hasSideEffects are unset
// by default, and TableGen will infer their value from the instruction
// pattern when possible.
//
// Normally, TableGen will issue an error it it can't infer the value of a
// property that hasn't been set explicitly. When guessInstructionProperties
// is set, it will guess a safe value instead.
//
// This option is a temporary migration help. It will go away.

Most notably, TableGen will infer hasSideEffects=1 for an instruction
with no pattern. Setting guessInstructionProperties=0 is a good way to
guard against this happening unexpectedly, but it hasn't actually been
possible to do so because a number of the TargetOpcode::* instructions
defined in Target.td relied on instruction properties being guessed. I
removed this limitation in r317674 [2] (making the previous inferred
properties explicit), while follow-up patches fix cases where
hasSideEffects=1 was inferred when that was not the intent (e.g. for
PHI[3], BUNDLE[4], CFI_INSTRUCTION/*_LABEL[5]).

So, how does this affect you? Should you be migrating your backend to
setting guessInstructionProperties=0? There is a small chance you
could see codegen changes in out-of-tree backends or passes
(especially if you were making use of the BUNDLE instruction, though
that patch has not yet landed). You should only see breakage if you
had code that was incorrectly assuming PHI or other instructions have
hasSideEffects set.

As for migrating your backend to guessInstructionProperties=0, this is
actually where I hope we can have a discussion. Setting
guessInstructionProperties=0 has helped to unearth bugs in the target
independent instructions. I've been using it in the RISC-V backend and
diligently setting mayLoad, mayStore and hasSideEffects explicitly
rather than setting blanket defaults. I'm now inclined to move to
setting hasSideEffects=0, mayLoad=0, mayStore=0 for the base RVInst
instruction. Although you will have to correctly set mayLoad=1 or
mayStore=1 for load/store instructions, this doesn't seem to be a
bigger burden than for the other properties that must be set correctly
(isBranch, setting Defs etc etc).

Thoughts and feedback welcome. Is moving to
guessInstructionProperties=0 by default still a worthwhile goal?

If anyone is motivated to see hasSideEffects=0 on BUNDLE, I'd
appreciate if you wanted to commandeer the patch[4] - it's fiddly to
check and update the changed test cases for someone unfamiliar with
AMDGPU.

[1]: https://reviews.llvm.org/rL162460
[2]: https://reviews.llvm.org/rL317674
[3]: https://reviews.llvm.org/rL317721
[4]: https://reviews.llvm.org/D37230
[5]: https://reviews.llvm.org/D39941

Best,

Alex